sql/inspect: clarify error message for internal failures

Previously, when INSPECT encountered any internal errors, it reported
them generically as "INSPECT found inconsistencies", regardless of
whether the issue was due to actual data corruption or internal
failures (e.g., invalid internal SQL). This could be misleading and
unnecessarily alarming.

This commit changes that behavior. If all errors seen by INSPECT are
internal, a new error message is returned: "INSPECT encountered
internal errors". This helps distinguish between data problems and
issues with the inspect code itself.

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,9 @@ 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 jobErr != nil &&
+		(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,64 @@ 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
+	foundIssue          atomic.Bool
+	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 {
+	l.foundIssue.Store(true)
+	if issue != 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.foundIssue.Load()
+}
+
+// sawOnlyInternalIssues reports whether every recorded issue was an internal error.
+func (l *inspectLoggerBundle) sawOnlyInternalIssues() bool {
+	if l == nil {
+		return false
+	}
+	return l.foundIssue.Load() && l.sawInternalIssue.Load() && !l.sawNonInternalIssue.Load()
+}
+
+// hasInternalIssues reports whether any internal errors were encountered.
+func (l *inspectLoggerBundle) hasInternalIssues() bool {
+	if l == nil {
+		return false
 	}
-	return false
+	return l.sawInternalIssue.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,
 	}