sql/inspect: clarify error message for internal failures

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

Author: spilchen

pkg/sql/inspect/BUILD.bazel

@@ -120,6 +120,7 @@ go_test(
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_cockroachdb_redact//:redact",
         "@com_github_gogo_protobuf//types",
+        "@com_github_lib_pq//:pq",
         "@com_github_stretchr_testify//require",
     ],
 )

pkg/sql/inspect/index_consistency_check_test.go

@@ -26,10 +26,14 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/errors"
 	"github.com/cockroachdb/redact"
+	"github.com/lib/pq"
 	"github.com/stretchr/testify/require"
 )
 
-const expectedInspectFoundInconsistencies = "INSPECT found inconsistencies"
+const (
+	expectedInspectFoundInconsistencies = "INSPECT found inconsistencies"
+	expectedInspectInternalErrors       = "INSPECT encountered internal errors"
+)
 
 // requireCheckCountsMatch verifies that the job's total check count equals its completed check count.
 // This is used to verify that progress tracking correctly counted all checks.
@@ -229,7 +233,7 @@ func TestDetectIndexConsistencyErrors(t *testing.T) {
 			expectedIssues: []inspectIssue{
 				{ErrorType: "internal_error"},
 			},
-			expectedErrRegex: expectedInspectFoundInconsistencies,
+			expectedErrRegex: expectedInspectInternalErrors,
 			expectedInternalErrorPatterns: []map[string]string{
 				{
 					"error_message": "error decoding.*float64",
@@ -463,6 +467,10 @@ func TestDetectIndexConsistencyErrors(t *testing.T) {
 
 			require.Error(t, err)
 			require.Regexp(t, tc.expectedErrRegex, err.Error())
+			var pqErr *pq.Error
+			require.True(t, errors.As(err, &pqErr), "expected pq.Error, got %T", err)
+			require.NotEmpty(t, pqErr.Hint, "expected error to have a hint")
+			require.Regexp(t, "SHOW INSPECT ERRORS FOR JOB [0-9]+ WITH DETAILS", pqErr.Hint)
 
 			numExpected := len(tc.expectedIssues)
 			numFound := issueLogger.numIssuesFound()

pkg/sql/inspect/inspect_job.go

@@ -105,8 +105,8 @@ func (c *inspectResumer) OnFailOrCancel(
 	execCfg := jobExecCtx.ExecCfg()
 	c.maybeCleanupProtectedTimestamp(ctx, execCfg)
 
-	// Record RunsWithIssues metric if the job failed due to finding inconsistencies.
-	if jobErr != nil && errors.Is(jobErr, errInspectFoundInconsistencies) {
+	// Record RunsWithIssues metric if the job failed due to finding issues (including internal errors).
+	if errors.Is(jobErr, errInspectFoundInconsistencies) || errors.Is(jobErr, errInspectInternalErrors) {
 		execCfg.JobRegistry.MetricsStruct().Inspect.(*InspectMetrics).RunsWithIssues.Inc(1)
 	}
 	return nil

pkg/sql/inspect/inspect_logger.go

@@ -23,29 +23,55 @@ type inspectLogger interface {
 	hasIssues() bool
 }
 
-// inspectLoggers manages a collection of inspectLogger instances.
-type inspectLoggers []inspectLogger
+// inspectLoggerBundle fan-outs logged inspect issues to a slice of loggers while
+// tracking aggregate state about the issues encountered.
+type inspectLoggerBundle struct {
+	loggers             []inspectLogger
+	sawInternalIssue    atomic.Bool
+	sawNonInternalIssue atomic.Bool
+}
 
-var _ inspectLogger = inspectLoggers{}
+var _ inspectLogger = (*inspectLoggerBundle)(nil)
 
-func (l inspectLoggers) logIssue(ctx context.Context, issue *inspectIssue) error {
-	var retErr error
+// newInspectLoggerBundle constructs a bundle that delegates to the provided loggers.
+func newInspectLoggerBundle(loggers ...inspectLogger) *inspectLoggerBundle {
+	return &inspectLoggerBundle{loggers: loggers}
+}
+
+// logIssue implements the inspectLogger interface.
+func (l *inspectLoggerBundle) logIssue(ctx context.Context, issue *inspectIssue) error {
+	if issue == nil {
+		return errors.AssertionFailedf("issue is nil")
+	}
+	if issue.ErrorType == InternalError {
+		l.sawInternalIssue.Store(true)
+	} else {
+		l.sawNonInternalIssue.Store(true)
+	}
 
-	for _, logger := range l {
+	var retErr error
+	for _, logger := range l.loggers {
 		if err := logger.logIssue(ctx, issue); err != nil {
 			retErr = errors.CombineErrors(retErr, err)
 		}
 	}
 	return retErr
 }
 
-func (l inspectLoggers) hasIssues() bool {
-	for _, logger := range l {
-		if logger.hasIssues() {
-			return true
-		}
+// hasIssues implements the inspectLogger interface.
+func (l *inspectLoggerBundle) hasIssues() bool {
+	if l == nil {
+		return false
+	}
+	return l.sawInternalIssue.Load() || l.sawNonInternalIssue.Load()
+}
+
+// sawOnlyInternalIssues reports whether every recorded issue was an internal error.
+func (l *inspectLoggerBundle) sawOnlyInternalIssues() bool {
+	if l == nil {
+		return false
 	}
-	return false
+	return l.sawInternalIssue.Load() && !l.sawNonInternalIssue.Load()
 }
 
 // metricsLogger increments metrics when issues are logged.

pkg/sql/inspect/inspect_metrics_test.go

@@ -17,6 +17,8 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/errors"
+	"github.com/lib/pq"
 	"github.com/stretchr/testify/require"
 )
 
@@ -75,6 +77,10 @@ func TestInspectMetrics(t *testing.T) {
 	_, err = db.Exec("INSPECT TABLE db.t")
 	require.Error(t, err, "INSPECT should fail when corruption is detected")
 	require.Contains(t, err.Error(), "INSPECT found inconsistencies")
+	var pqErr *pq.Error
+	require.True(t, errors.As(err, &pqErr), "expected pq.Error, got %T", err)
+	require.NotEmpty(t, pqErr.Hint, "expected error to have a hint")
+	require.Regexp(t, "SHOW INSPECT ERRORS FOR JOB [0-9]+ WITH DETAILS", pqErr.Hint)
 	require.Equal(t, initialRuns+2, metrics.Runs.Count(),
 		"Runs counter should increment for each job execution")
 	require.Equal(t, initialRunsWithIssues+1, metrics.RunsWithIssues.Count(),

pkg/sql/inspect/inspect_processor.go

@@ -34,6 +34,8 @@ var (
 	// errInspectFoundInconsistencies is a sentinel error used to mark errors
 	// returned when INSPECT jobs find data inconsistencies.
 	errInspectFoundInconsistencies = pgerror.New(pgcode.DataException, "INSPECT found inconsistencies")
+	// errInspectInternalErrors marks jobs that only encountered internal errors while running.
+	errInspectInternalErrors = pgerror.New(pgcode.Internal, "INSPECT encountered internal errors")
 
 	processorConcurrencyOverride = settings.RegisterIntSetting(
 		settings.ApplicationLevel,
@@ -87,7 +89,7 @@ type inspectProcessor struct {
 	cfg            *execinfra.ServerConfig
 	checkFactories []inspectCheckFactory
 	spanSrc        spanSource
-	logger         inspectLogger
+	loggerBundle   *inspectLoggerBundle
 	concurrency    int
 	clock          *hlc.Clock
 	mu             struct {
@@ -203,9 +205,17 @@ func (p *inspectProcessor) runInspect(ctx context.Context, output execinfra.RowR
 		return err
 	}
 
-	log.Dev.Infof(ctx, "INSPECT processor completed processorID=%d issuesFound=%t", p.processorID, p.logger.hasIssues())
-	if p.logger.hasIssues() {
-		return errInspectFoundInconsistencies
+	log.Dev.Infof(ctx, "INSPECT processor completed processorID=%d issuesFound=%t", p.processorID, p.loggerBundle.hasIssues())
+	if p.loggerBundle.hasIssues() {
+		errToReturn := errInspectFoundInconsistencies
+		if p.loggerBundle.sawOnlyInternalIssues() {
+			errToReturn = errInspectInternalErrors
+		}
+		return errors.WithHintf(
+			errToReturn,
+			"Run 'SHOW INSPECT ERRORS FOR JOB %d WITH DETAILS' for more information.",
+			p.spec.JobID,
+		)
 	}
 	return nil
 }
@@ -221,12 +231,12 @@ func getProcessorConcurrency(flowCtx *execinfra.FlowCtx) int {
 	return runtime.GOMAXPROCS(0)
 }
 
-// getInspectLogger returns a logger for the inspect processor.
-func getInspectLogger(flowCtx *execinfra.FlowCtx, jobID jobspb.JobID) inspectLogger {
+// getInspectLogger returns a logger bundle for the inspect processor.
+func getInspectLogger(flowCtx *execinfra.FlowCtx, jobID jobspb.JobID) *inspectLoggerBundle {
 	execCfg := flowCtx.Cfg.ExecutorConfig.(*sql.ExecutorConfig)
 	metrics := execCfg.JobRegistry.MetricsStruct().Inspect.(*InspectMetrics)
 
-	loggers := inspectLoggers{
+	loggers := []inspectLogger{
 		&logSink{},
 		&tableSink{
 			db:    flowCtx.Cfg.DB,
@@ -243,7 +253,7 @@ func getInspectLogger(flowCtx *execinfra.FlowCtx, jobID jobspb.JobID) inspectLog
 		loggers = append(loggers, knobs.InspectIssueLogger.(inspectLogger))
 	}
 
-	return loggers
+	return newInspectLoggerBundle(loggers...)
 }
 
 // processSpan executes all configured inspect checks against a single span.
@@ -270,7 +280,7 @@ func (p *inspectProcessor) processSpan(
 
 	runner := inspectRunner{
 		checks: checks,
-		logger: p.logger,
+		logger: p.loggerBundle,
 	}
 	defer func() {
 		if closeErr := runner.Close(ctx); closeErr != nil {
@@ -412,7 +422,7 @@ func newInspectProcessor(
 		checkFactories: checkFactories,
 		cfg:            flowCtx.Cfg,
 		spanSrc:        newSliceSpanSource(spec.Spans),
-		logger:         getInspectLogger(flowCtx, spec.JobID),
+		loggerBundle:   getInspectLogger(flowCtx, spec.JobID),
 		concurrency:    getProcessorConcurrency(flowCtx),
 		clock:          flowCtx.Cfg.DB.KV().Clock(),
 	}, nil

pkg/sql/inspect/inspect_processor_test.go

@@ -257,6 +257,7 @@ func makeProcessor(
 	t.Helper()
 	clock := hlc.NewClockForTesting(nil)
 	logger := &testIssueCollector{}
+	loggerBundle := newInspectLoggerBundle(logger)
 
 	// Mock a FlowCtx for test purposes.
 	var c base.NodeIDContainer
@@ -277,7 +278,7 @@ func makeProcessor(
 		cfg:            flowCtx.Cfg,
 		flowCtx:        flowCtx,
 		spanSrc:        src,
-		logger:         logger,
+		loggerBundle:   loggerBundle,
 		concurrency:    concurrency,
 		clock:          clock,
 	}