Merge #155837

155837: sql/inspect: fix AS OF SYSTEM TIME span bounds in index checks r=spilchen a=spilchen

INSPECT's index consistency checks could miss spans when using AS OF SYSTEM TIME if those spans had no data at the current time but did at the historical timestamp. This was due to SpanToQueryBounds() using the current time for KV scans instead of the historical time.

This commit:
- Adds an asOf parameter to SpanToQueryBounds() for historical scans
- Updates INSPECT to pass the query's timestamp
- Keeps existing behavior for TTL and tests by passing an empty timestamp

The issue was uncovered while converting TestScrubIndexMissingIndexEntry to INSPECT. The test is now TestMissingIndexEntryWithHistoricalQuery and validates correctness of historical scans.

Informs: #155485
Epic: CRDB-55075

Release note (bug fix): INSPECT now correctly checks index consistency at the historical timestamp when using AS OF SYSTEM TIME, even for spans with no current data.

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

Author: craig[bot]

pkg/sql/inspect/index_consistency_check.go

@@ -168,7 +168,7 @@ func (c *indexConsistencyCheck) Start(
 	alloc := &tree.DatumAlloc{}
 	bounds, hasRows, err := spanutils.SpanToQueryBounds(
 		ctx, cfg.DB.KV(), cfg.Codec, pkColIDs, pkColTypes, pkColDirs,
-		len(c.tableDesc.GetFamilies()), span, alloc,
+		len(c.tableDesc.GetFamilies()), span, alloc, c.asOf,
 	)
 	if err != nil {
 		return errors.Wrap(err, "converting span to query bounds")

pkg/sql/inspect/index_consistency_check_test.go

@@ -614,3 +614,70 @@ func TestIndexConsistencyWithReservedWordColumns(t *testing.T) {
 	require.InEpsilon(t, 1.0, fractionCompleted, 0.01, "progress should reach 100%% on successful completion")
 	requireCheckCountsMatch(t, r, jobID)
 }
+
+// TestMissingIndexEntryWithHistoricalQuery verifies that INSPECT can detect
+// missing index entries when querying at a historical timestamp (AS OF SYSTEM
+// TIME), including cases where data has since been deleted.
+//
+// This test overlaps with TestDetectIndexConsistencyErrors but is kept to
+// expand coverage. It exercises scenarios that TestDetectIndexConsistencyErrors
+// missed and specifically validates INSPECT’s handling of AS OF SYSTEM TIME and
+// mixed-case table names.
+func TestMissingIndexEntryWithHistoricalQuery(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+	ctx := context.Background()
+	s, db, kvDB := serverutils.StartServer(t, base.TestServerArgs{})
+	defer s.Stopper().Stop(ctx)
+	r := sqlutils.MakeSQLRunner(db)
+	codec := s.ApplicationLayer().Codec()
+
+	// Create the table and the row entry.
+	// We use a table with mixed case as a regression case for #38184.
+	r.ExecMultiple(t,
+		`CREATE DATABASE t`,
+		`CREATE TABLE t."tEst" ("K" INT PRIMARY KEY, v INT)`,
+		`CREATE INDEX secondary ON t."tEst" (v)`,
+		`INSERT INTO t."tEst" VALUES (10, 20)`,
+	)
+
+	// Construct datums for our row values (k, v).
+	values := []tree.Datum{tree.NewDInt(10), tree.NewDInt(20)}
+	tableDesc := desctestutils.TestingGetPublicTableDescriptor(kvDB, codec, "t", "tEst")
+	secondaryIndex := tableDesc.PublicNonPrimaryIndexes()[0]
+
+	// Delete the secondary index entry to create corruption.
+	err := deleteSecondaryIndexEntry(ctx, codec, values, kvDB, tableDesc, secondaryIndex)
+	require.NoError(t, err)
+
+	// Enable INSPECT command.
+	r.Exec(t, `SET enable_inspect_command = true`)
+
+	// Run INSPECT and expect it to find the missing index entry.
+	// INSPECT returns an error when inconsistencies are found.
+	_, err = db.Query(`INSPECT TABLE t."tEst" WITH OPTIONS INDEX ALL`)
+	require.Error(t, err, "expected INSPECT to return an error when inconsistencies are found")
+	require.Contains(t, err.Error(), "INSPECT found inconsistencies")
+
+	// Run again with AS OF SYSTEM TIME.
+	time.Sleep(1 * time.Millisecond)
+	_, err = db.Query(`INSPECT TABLE t."tEst" AS OF SYSTEM TIME '-1ms' WITH OPTIONS INDEX ALL`)
+	require.Error(t, err, "expected INSPECT to return an error when inconsistencies are found")
+	require.Contains(t, err.Error(), "INSPECT found inconsistencies")
+
+	// Verify that AS OF SYSTEM TIME actually operates in the past by:
+	// 1. Getting a timestamp before we delete the row
+	// 2. Deleting the entire row
+	// 3. Running INSPECT at the historical timestamp
+	// At that historical timestamp, the row existed and was corrupted, so INSPECT
+	// should still find the corruption even though the row no longer exists.
+	ts := r.QueryStr(t, `SELECT cluster_logical_timestamp()`)[0][0]
+	r.Exec(t, `DELETE FROM t."tEst"`)
+
+	_, err = db.Query(fmt.Sprintf(
+		`INSPECT TABLE t."tEst" AS OF SYSTEM TIME '%s' WITH OPTIONS INDEX ALL`, ts,
+	))
+	require.Error(t, err, "expected INSPECT to find corruption at historical timestamp")
+	require.Contains(t, err.Error(), "INSPECT found inconsistencies",
+		"INSPECT should detect the missing index entry that existed at the historical timestamp")
+}

pkg/sql/scrub_test.go

@@ -30,64 +30,6 @@ import (
 	"github.com/cockroachdb/errors"
 )
 
-// TestScrubIndexMissingIndexEntry tests that
-// `SCRUB TABLE ... INDEX ALL“ will find missing index entries. To test
-// this, a row's underlying secondary index k/v is deleted using the KV
-// client. This causes a missing index entry error as the row is missing
-// the expected secondary index k/v.
-func TestScrubIndexMissingIndexEntry(t *testing.T) {
-	defer leaktest.AfterTest(t)()
-	defer log.Scope(t).Close(t)
-	s, db, kvDB := serverutils.StartServer(t, base.TestServerArgs{})
-	defer s.Stopper().Stop(context.Background())
-	r := sqlutils.MakeSQLRunner(db)
-
-	// Create the table and the row entry.
-	// We use a table with mixed as a regression case for #38184.
-	if _, err := db.Exec(`
-CREATE DATABASE t;
-CREATE TABLE t."tEst" ("K" INT PRIMARY KEY, v INT);
-CREATE INDEX secondary ON t."tEst" (v);
-INSERT INTO t."tEst" VALUES (10, 20);
-`); err != nil {
-		t.Fatalf("unexpected error: %s", err)
-	}
-
-	// Construct datums for our row values (k, v).
-	values := []tree.Datum{tree.NewDInt(10), tree.NewDInt(20)}
-	tableDesc := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "t", "tEst")
-	secondaryIndex := tableDesc.PublicNonPrimaryIndexes()[0]
-	if err := removeIndexEntryForDatums(values, kvDB, tableDesc, secondaryIndex); err != nil {
-		t.Fatalf("unexpected error: %s", err.Error())
-	}
-
-	// Run SCRUB and find the index errors we created.
-	exp := []scrubtestutils.ExpectedScrubResult{
-		{
-			ErrorType:    scrub.MissingIndexEntryError,
-			Database:     "t",
-			Table:        "tEst",
-			PrimaryKey:   "(10)",
-			Repaired:     false,
-			DetailsRegex: `"v": "20"`,
-		},
-	}
-	scrubtestutils.RunScrub(t, db, `EXPERIMENTAL SCRUB TABLE t."tEst" WITH OPTIONS INDEX ALL`, exp)
-	// Run again with AS OF SYSTEM TIME.
-	time.Sleep(1 * time.Millisecond)
-	scrubtestutils.RunScrub(t, db, `EXPERIMENTAL SCRUB TABLE t."tEst" AS OF SYSTEM TIME '-1ms' WITH OPTIONS INDEX ALL`, exp)
-
-	// Verify that AS OF SYSTEM TIME actually operates in the past.
-	ts := r.QueryStr(t, `SELECT cluster_logical_timestamp()`)[0][0]
-	r.Exec(t, `DELETE FROM t."tEst"`)
-	scrubtestutils.RunScrub(
-		t, db, fmt.Sprintf(
-			`EXPERIMENTAL SCRUB TABLE t."tEst" AS OF SYSTEM TIME '%s' WITH OPTIONS INDEX ALL`, ts,
-		),
-		exp,
-	)
-}
-
 // TestScrubIndexPartialIndex tests that SCRUB catches various anomalies in the data contained in a
 // partial secondary index.
 func TestScrubIndexPartialIndex(t *testing.T) {

pkg/sql/spanutils/BUILD.bazel

@@ -15,6 +15,7 @@ go_library(
         "//pkg/sql/rowenc",
         "//pkg/sql/sem/tree",
         "//pkg/sql/types",
+        "//pkg/util/hlc",
         "@com_github_cockroachdb_errors//:errors",
     ],
 )
@@ -45,6 +46,7 @@ go_test(
         "//pkg/testutils/skip",
         "//pkg/testutils/sqlutils",
         "//pkg/testutils/testcluster",
+        "//pkg/util/hlc",
         "//pkg/util/leaktest",
         "//pkg/util/log",
         "@com_github_stretchr_testify//require",

pkg/sql/spanutils/query_bounds.go

@@ -19,6 +19,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/rowenc"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
+	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/errors"
 )
 
@@ -84,18 +85,46 @@ func SpanToQueryBounds(
 	numFamilies int,
 	span roachpb.Span,
 	alloc *tree.DatumAlloc,
+	asOf hlc.Timestamp,
 ) (bounds QueryBounds, hasRows bool, _ error) {
 	partialStartKey := span.Key
 	partialEndKey := span.EndKey
-	startKeyValues, err := kvDB.Scan(ctx, partialStartKey, partialEndKey, int64(numFamilies))
+
+	var startKeyValues []kv.KeyValue
+	var err error
+	if asOf.IsEmpty() {
+		startKeyValues, err = kvDB.Scan(ctx, partialStartKey, partialEndKey, int64(numFamilies))
+	} else {
+		err = kvDB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
+			if err := txn.SetFixedTimestamp(ctx, asOf); err != nil {
+				return err
+			}
+			var scanErr error
+			startKeyValues, scanErr = txn.Scan(ctx, partialStartKey, partialEndKey, int64(numFamilies))
+			return scanErr
+		})
+	}
 	if err != nil {
 		return bounds, false, errors.Wrapf(err, "scan error startKey=%x endKey=%x", []byte(partialStartKey), []byte(partialEndKey))
 	}
 	// If span has 0 rows then return early - it will not be processed.
 	if len(startKeyValues) == 0 {
 		return bounds, false, nil
 	}
-	endKeyValues, err := kvDB.ReverseScan(ctx, partialStartKey, partialEndKey, int64(numFamilies))
+
+	var endKeyValues []kv.KeyValue
+	if asOf.IsEmpty() {
+		endKeyValues, err = kvDB.ReverseScan(ctx, partialStartKey, partialEndKey, int64(numFamilies))
+	} else {
+		err = kvDB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
+			if err := txn.SetFixedTimestamp(ctx, asOf); err != nil {
+				return err
+			}
+			var scanErr error
+			endKeyValues, scanErr = txn.ReverseScan(ctx, partialStartKey, partialEndKey, int64(numFamilies))
+			return scanErr
+		})
+	}
 	if err != nil {
 		return bounds, false, errors.Wrapf(err, "reverse scan error startKey=%x endKey=%x", []byte(partialStartKey), []byte(partialEndKey))
 	}

pkg/sql/spanutils/query_bounds_test.go

@@ -23,6 +23,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
+	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/stretchr/testify/require"
@@ -202,7 +203,7 @@ func TestSpanToQueryBounds(t *testing.T) {
 			actualBounds, actualHasRows, err := spanutils.SpanToQueryBounds(ctx, kvDB, codec, pkColIDs, pkColTypes, pkColDirs, 1, roachpb.Span{
 				Key:    startKey,
 				EndKey: endKey,
-			}, &alloc)
+			}, &alloc, hlc.Timestamp{})
 
 			// Verify results.
 			require.NoError(t, err)
@@ -423,6 +424,7 @@ func TestSpanToQueryBoundsCompositeKeys(t *testing.T) {
 						EndKey: endKey,
 					},
 					&alloc,
+					hlc.Timestamp{},
 				)
 
 				// Verify results.

pkg/sql/ttl/ttljob/BUILD.bazel

@@ -48,6 +48,7 @@ go_library(
         "//pkg/sql/types",
         "//pkg/util/admission/admissionpb",
         "//pkg/util/ctxgroup",
+        "//pkg/util/hlc",
         "//pkg/util/log",
         "//pkg/util/metric",
         "//pkg/util/metric/aggmetric",

pkg/sql/ttl/ttljob/ttljob_processor.go

@@ -37,6 +37,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/cockroach/pkg/util/admission/admissionpb"
 	"github.com/cockroachdb/cockroach/pkg/util/ctxgroup"
+	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/quotapool"
 	"github.com/cockroachdb/cockroach/pkg/util/retry"
@@ -356,6 +357,7 @@ func (t *ttlProcessor) work(ctx context.Context, output execinfra.RowReceiver) e
 				numFamilies,
 				span,
 				&alloc,
+				hlc.Timestamp{}, // Use current time bounds; TTL only deletes live rows.
 			); err != nil {
 				return errors.Wrapf(err, "SpanToQueryBounds error index=%d span=%s", i, span)
 			} else if hasRows {