Merge #155766

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

Co-authored-by: Matt Spilchen <matt.spilchen@cockroachlabs.com>

Author: craig[bot]

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.
@@ -231,7 +235,7 @@ func TestDetectIndexConsistencyErrors(t *testing.T) {
 			expectedIssues: []inspectIssue{
 				{ErrorType: "internal_error"},
 			},
-			expectedErrRegex: expectedInspectFoundInconsistencies,
+			expectedErrRegex: expectedInspectInternalErrors,
 			expectedInternalErrorPatterns: []map[string]string{
 				{
 					"error_message": "error decoding.*float64",
@@ -465,6 +469,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"
 )
 
@@ -74,6 +76,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,
 	}