sql/inspect: clarify error message for internal failures #155766

spilchen · 2025-10-21T13:07:19Z

Previously, when INSPECT encountered internal errors, it always reported "INSPECT found inconsistencies," even if the issue was caused by something other than actual data corruption. This made it hard to tell whether the problem was in user data or within INSPECT itself (for example, a bad query generated internally).

This commit refines that behavior. If all observed errors are internal, INSPECT now reports: "INSPECT encountered internal errors." This makes it clear that the problem might stem from an internal failure in INSPECT or from data corruption detected during internal queries, rather than always implying user data inconsistencies.

Additionally, a hint is now included in the error message, regardless of error type, guiding users to run SHOW INSPECT ERRORS to retrieve more information.

Informs: #155676
Epic: CRDB-55075

Release note: none

cockroach-teamcity · 2025-10-21T13:07:31Z

This change is

rafiss · 2025-10-21T16:12:49Z

pkg/sql/inspect/inspect_job.go

-	if jobErr != nil && errors.Is(jobErr, errInspectFoundInconsistencies) {
+	// Record RunsWithIssues metric if the job failed due to finding issues (including internal errors).
+	if jobErr != nil &&
+		(errors.Is(jobErr, errInspectFoundInconsistencies) || errors.Is(jobErr, errInspectInternalErrors)) {


super nit: errors.Is already has a nil check, so we can skip that explicit check here.

rafiss · 2025-10-21T16:15:06Z

pkg/sql/inspect/inspect_logger.go

+// tracking aggregate state about the issues encountered.
+type inspectLoggerBundle struct {
+	loggers             []inspectLogger
+	foundIssue          atomic.Bool


nit: do we need foundIssue? it seems like we can always derive it from sawInternalIssue || sawNonInternalIssue

rafiss · 2025-10-21T16:19:05Z

pkg/sql/inspect/inspect_logger.go

+func (l *inspectLoggerBundle) logIssue(ctx context.Context, issue *inspectIssue) error {
+	l.foundIssue.Store(true)
+	if issue != nil {
+		if issue.ErrorType == InternalError {


making sure I follow -- the comment on InternalError says

// InternalError occurs when an internal error (e.g., data corruption, encoding // issues) prevents the check from completing normally. These errors indicate // potential data corruption or other serious issues that require investigation.

but the commit message here says these are issues with the inspect code itself, and are not data problems.

is it just a matter of updating the comment, or do we need to account for other important cases where InternalError is used?

It's good to call this out. The answer is that it depends. If you have a corruption such that it causes bad things to happen in the query engine, it will manifest as an internal error. But you can also have errors where the SQL crafted by INSPECT isn't valid. An example of that is seen in #155772: you cannot have an equality comparison between column of types REFCURSOR.

Let me update the commit/PR description.

Previously, when INSPECT encountered internal errors, it always reported "INSPECT found inconsistencies," even if the issue was caused by something other than actual data corruption. This made it hard to tell whether the problem was in user data or within INSPECT itself (for example, a bad query generated internally). This commit refines that behavior. If all observed errors are internal, INSPECT now reports: "INSPECT encountered internal errors." This makes it clear that the problem might stem from an internal failure in INSPECT or from data corruption detected during internal queries, rather than always implying user data inconsistencies. Additionally, a hint is now included in the error message, regardless of error type, guiding users to run SHOW INSPECT ERRORS to retrieve more information. Informs: cockroachdb#155676 Epic: CRDB-55075 Release note: none

spilchen

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @spilchen)

spilchen · 2025-10-21T17:15:19Z

pkg/sql/inspect/inspect_job.go

-	if jobErr != nil && errors.Is(jobErr, errInspectFoundInconsistencies) {
+	// Record RunsWithIssues metric if the job failed due to finding issues (including internal errors).
+	if jobErr != nil &&
+		(errors.Is(jobErr, errInspectFoundInconsistencies) || errors.Is(jobErr, errInspectInternalErrors)) {


spilchen · 2025-10-21T17:15:19Z

pkg/sql/inspect/inspect_logger.go

+// tracking aggregate state about the issues encountered.
+type inspectLoggerBundle struct {
+	loggers             []inspectLogger
+	foundIssue          atomic.Bool


spilchen · 2025-10-21T17:15:20Z

pkg/sql/inspect/inspect_logger.go

+func (l *inspectLoggerBundle) logIssue(ctx context.Context, issue *inspectIssue) error {
+	l.foundIssue.Store(true)
+	if issue != nil {
+		if issue.ErrorType == InternalError {


It's good to call this out. The answer is that it depends. If you have a corruption such that it causes bad things to happen in the query engine, it will manifest as an internal error. But you can also have errors where the SQL crafted by INSPECT isn't valid. An example of that is seen in #155772: you cannot have an equality comparison between column of types REFCURSOR.

Let me update the commit/PR description.

rafiss

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @spilchen)

spilchen · 2025-10-22T19:48:06Z

TFTR!

bors r+

155766: sql/inspect: clarify error message for internal failures r=spilchen a=spilchen Previously, when `INSPECT` encountered internal errors, it always reported "INSPECT found inconsistencies," even if the issue was caused by something other than actual data corruption. This made it hard to tell whether the problem was in user data or within INSPECT itself (for example, a bad query generated internally). This commit refines that behavior. If all observed errors are internal, `INSPECT` now reports: "INSPECT encountered internal errors." This makes it clear that the problem might stem from an internal failure in INSPECT or from data corruption detected during internal queries, rather than always implying user data inconsistencies. Additionally, a hint is now included in the error message, regardless of error type, guiding users to run SHOW INSPECT ERRORS to retrieve more information. Informs: #155676 Epic: CRDB-55075 Release note: none 155824: sql: fix top-level query stats when "inner" plans are present r=yuzefovich a=yuzefovich Previously, whenever we ran an "inner" plan (via `runPlanInsidePlan` helper), we ignored the top-level query stats for the "inner" plan. As a result, reads and writes performed by the inner plan weren't reflected in the "outer" top-level query stats. This is now fixed by adjusting the routines, apply joins, and recursive CTEs to propagate their metrics as ProducerMetadata objects. Note that for routines the access to the DistSQL infra is rather difficult, so we plumbed the access via the planner straight into the DistSQLReceiver, and even though it's ugly, it should work in practice. The only alternative I see is adding the necessary reference into the `eval.Context`, but then it gets tricky to actually set that and ensure the right copy of the eval context is observed by all routines (plus we'd need to make the copy or restore the original state somehow), so I chose to not pursue it. Epic: None Release note (bug fix): Previously, CockroachDB didn't include reads and writes performed by routines (user-defined functions and stored procedures) as well as apply joins into `bytes read`, `rows read`, and `rows written` statement execution statistics, and this is now fixed. The bug has been present since before 23.2 version. Co-authored-by: Matt Spilchen <[email protected]> Co-authored-by: Yahor Yuzefovich <[email protected]>

craig · 2025-10-22T21:04:16Z

Build failed (retrying...):

local_roachtest_fips

craig · 2025-10-22T22:49:04Z

Build succeeded:

spilchen self-assigned this Oct 21, 2025

spilchen force-pushed the gh-155484/251021/0840/inspect-refcursor/pr-ready branch from a2f463e to 5d723f5 Compare October 21, 2025 13:59

spilchen marked this pull request as ready for review October 21, 2025 14:00

spilchen requested a review from a team as a code owner October 21, 2025 14:00

spilchen requested a review from rafiss October 21, 2025 14:00

rafiss requested changes Oct 21, 2025

View reviewed changes

spilchen force-pushed the gh-155484/251021/0840/inspect-refcursor/pr-ready branch from 5d723f5 to 84ea39c Compare October 21, 2025 17:15

spilchen commented Oct 21, 2025

View reviewed changes

spilchen requested a review from rafiss October 21, 2025 18:56

rafiss approved these changes Oct 22, 2025

View reviewed changes

craig bot merged commit 8a58a96 into cockroachdb:master Oct 22, 2025
23 of 24 checks passed

celeste-cockroachdb bot added the target-release-26.1.0 label Oct 22, 2025

rishabh7m added a commit to crltest-issue-syncing/cockroach-fork that referenced this pull request Nov 20, 2025

Add simulation README for PR cockroachdb#155766

b430628

rishabh7m added a commit to crltest-issue-syncing/cockroach-fork that referenced this pull request Nov 21, 2025

Add simulation README for PR cockroachdb#155766

f648d3b

rishabh7m added a commit to crltest-issue-syncing/cockroach-fork that referenced this pull request Nov 21, 2025

Add simulation README for PR cockroachdb#155766

39a3abc

sql/inspect: clarify error message for internal failures #155766

sql/inspect: clarify error message for internal failures #155766

Uh oh!

Conversation

spilchen commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Oct 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spilchen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

spilchen commented Oct 22, 2025

Uh oh!

craig bot commented Oct 22, 2025

Uh oh!

craig bot commented Oct 22, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

spilchen commented Oct 21, 2025 •

edited

Loading