Merge #153716 #154539 #154646 #154782 #154861 #154868

153716: optbuilder: remove assertion annotation in an edge case r=yuzefovich a=yuzefovich

In 7ca00cc we added a panic-catcher to make any errors coming from `resolveAndRequireType` around partial indexes to be assertion failures. This was done under the assumption that we never expect any errors; however, we just saw a sentry report that had an error there that seems somewhat expected "interrupted during singleflight ×: context canceled". Thus, this commit removes the assertion annotation.

Fixes: #153634.
Release note: None

154539: explain: harden gist decoding logic for invalid gists r=yuzefovich a=yuzefovich

Our randomized test just encountered a couple of cases when an invalid gist resulted in an internal error when being decoded. The problem is that in such case we can have different fields of `Node`s left unset, leading to nil pointers or index-out-of-bounds. This commit hardens the code against such scenarios.

We did something similar in a561201.

Fixes: #154300.
Release note: None

154646: sql/importer: make inspect after import validation a metamorphic setting r=spilchen a=spilchen

Previously, the setting `bulkio.import.row_count_validation.unsafe.enabled` was a boolean that controlled whether an INSPECT job would be triggered after an IMPORT operation. This commit replaces that boolean with a metamorphic enum setting.

The new enum has three values:
- `off`: No validation (default; preserves current behavior)
- `async`: Run INSPECT asynchronously in the background (future production default)
- `sync`: Run INSPECT synchronously and wait for completion (testing only)

The prior `true` value now maps to `async`. The new `sync` option is added specifically for testing. If the INSPECT job fails, it causes the IMPORT to fail as well, enabling roachtests to detect issues more easily without manual validation.

The setting has been renamed to `bulkio.import.row_count_validation.unsafe.mode` to reflect the new setting.

Informs: #154049
Epic: CRDB-30356

Release note: none

154782: util: prioritize cancellations in retry loop r=kev-cao,yuzefovich a=sanki92

This commit teaches `util.Retry` to prioritize context cancellations and stoppers over retry attempts. This ensures more consistent behaviors and reduces test flakes.

Fixes: #154764

Release note: None

154861: kvnemesis: disable DRPC for tests r=shubhamdhama a=shubhamdhama

The 'TestKVNemesisMultiNode*' tests are showing flakiness when DRPC is enabled, and this can be reproduced under stress. Until the root cause is found, it's best to disable DRPC for these tests.

Release note: none
Epic: none
Informs: #154847

154868: backfill: skip TestVectorIndexMergingDuringBackfillWithPrefix under race r=yuzefovich a=yuzefovich

We've seen this recently added test time out twice under race with no clear signs of anything going wrong. The test seems just rather intense to be run under race, so we'll skip it in that config.

Fixes: #154844.
Release note: None

Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Matt Spilchen <[email protected]>
Co-authored-by: sanki92 <[email protected]>
Co-authored-by: Shubham Dhama <[email protected]>

Author: 5 people

pkg/kv/kvnemesis/main_test.go

@@ -30,7 +30,7 @@ func TestMain(m *testing.M) {
 	)()
 
 	defer serverutils.TestingGlobalDRPCOption(
-		base.TestDRPCEnabledRandomly,
+		base.TestDRPCDisabled,
 	)()
 
 	os.Exit(m.Run())

pkg/sql/backfill/BUILD.bazel

@@ -87,6 +87,7 @@ go_test(
         "//pkg/sql/sem/catid",
         "//pkg/sql/sem/eval",
         "//pkg/testutils/serverutils",
+        "//pkg/testutils/skip",
         "//pkg/testutils/sqlutils",
         "//pkg/testutils/testcluster",
         "//pkg/util/leaktest",

pkg/sql/backfill/backfill_test.go

@@ -14,6 +14,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/sql/execinfra"
 	"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/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -351,6 +352,8 @@ func TestVectorIndexMergingDuringBackfillWithPrefix(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
+	skip.UnderRace(t, "too slow")
+
 	// Channel to block the backfill process
 	blockBackfill := make(chan struct{})
 	backfillBlocked := make(chan struct{})

pkg/sql/catalog/descriptor.go

@@ -515,6 +515,9 @@ type TableDescriptor interface {
 	// keys, even if they're not defined by the user.
 	HasPrimaryKey() bool
 
+	// IsExpressionIndex returns if the given index is an expression index.
+	IsExpressionIndex(idx Index) bool
+
 	// AllColumns returns a slice of Column interfaces containing the
 	// table's public columns and column mutations, in the canonical order:
 	// - all public columns in the same order as in the underlying

pkg/sql/catalog/tabledesc/structured.go

@@ -2236,6 +2236,18 @@ func (desc *wrapper) HasPrimaryKey() bool {
 	return !desc.PrimaryIndex.Disabled
 }
 
+// IsExpressionIndex implements the TableDescriptor interface.
+func (desc *wrapper) IsExpressionIndex(idx catalog.Index) bool {
+	for i := 0; i < idx.NumKeyColumns(); i++ {
+		colID := idx.GetKeyColumnID(i)
+		col := catalog.FindColumnByID(desc, colID)
+		if col != nil && col.IsExpressionIndexColumn() {
+			return true
+		}
+	}
+	return false
+}
+
 // HasColumnBackfillMutation implements the TableDescriptor interface.
 func (desc *wrapper) HasColumnBackfillMutation() bool {
 	for _, m := range desc.AllMutations() {

pkg/sql/importer/BUILD.bazel

@@ -88,6 +88,7 @@ go_library(
         "//pkg/util/log",
         "//pkg/util/log/eventpb",
         "//pkg/util/log/logutil",
+        "//pkg/util/metamorphic",
         "//pkg/util/protoutil",
         "//pkg/util/retry",
         "//pkg/util/syncutil",

pkg/sql/importer/import_job.go

@@ -39,6 +39,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
 	"github.com/cockroachdb/cockroach/pkg/util/log/logutil"
+	"github.com/cockroachdb/cockroach/pkg/util/metamorphic"
 	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/cockroach/pkg/util/retry"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
@@ -106,16 +107,44 @@ var performConstraintValidation = settings.RegisterBoolSetting(
 	settings.WithUnsafe,
 )
 
-// TODO(janexing): tune the default to True when INSPECT is merged in stable release.
-var importRowCountValidation = settings.RegisterBoolSetting(
+// ImportRowCountValidationMode represents the mode for row count validation after import.
+type ImportRowCountValidationMode int64
+
+const (
+	// ImportRowCountValidationOff disables row count validation after import.
+	ImportRowCountValidationOff ImportRowCountValidationMode = iota
+	// ImportRowCountValidationAsync enables asynchronous row count validation after import.
+	ImportRowCountValidationAsync
+	// ImportRowCountValidationSync enables synchronous (blocking) row count validation after import.
+	ImportRowCountValidationSync
+)
+
+// importRowCountValidationMetamorphicValue determines the default value for
+// importRowCountValidation in metamorphic test builds. It randomly selects
+// between "off", "async", and "sync" modes to increase test coverage.
+var importRowCountValidationMetamorphicValue = metamorphic.ConstantWithTestChoice(
+	"import-row-count-validation",
+	"off",   // no validation
+	"async", // background validation
+	"sync",  // blocking validation for tests
+)
+
+// TODO(janexing): tune the default to async when INSPECT is merged in stable release.
+var importRowCountValidation = settings.RegisterEnumSetting(
 	settings.ApplicationLevel,
-	"bulkio.import.row_count_validation.unsafe.enabled",
-	"enables asynchronous validation of imported data via INSPECT "+
-		"jobs. When enabled, an INSPECT job runs after import completion to "+
-		"detect potential data corruption. Disabling this setting may result "+
-		"in undetected data corruption if the import process fails.",
-	false,
+	"bulkio.import.row_count_validation.unsafe.mode",
+	"controls validation of imported data via INSPECT jobs. "+
+		"Options: 'off' (no validation), 'async' (background validation), "+
+		"'sync' (blocking validation). "+
+		"If disabled, IMPORT will not perform a post-import row count check.",
+	importRowCountValidationMetamorphicValue,
+	map[ImportRowCountValidationMode]string{
+		ImportRowCountValidationOff:   "off",
+		ImportRowCountValidationAsync: "async",
+		ImportRowCountValidationSync:  "sync",
+	},
 	settings.WithUnsafe,
+	settings.WithRetiredName("bulkio.import.row_count_validation.unsafe.enabled"),
 )
 
 func getTable(details jobspb.ImportDetails) (jobspb.ImportDetails_Table, error) {
@@ -288,14 +317,18 @@ func (r *importResumer) Resume(ctx context.Context, execCtx interface{}) error {
 		return err
 	}
 
-	if importRowCountValidation.Get(&p.ExecCfg().Settings.SV) {
+	validationMode := importRowCountValidation.Get(&p.ExecCfg().Settings.SV)
+	switch validationMode {
+	case ImportRowCountValidationOff:
+	// No validation required.
+	case ImportRowCountValidationAsync, ImportRowCountValidationSync:
 		table, err := getTable(details)
 		if err != nil {
 			return err
 		}
 		tblDesc := tabledesc.NewBuilder(table.Desc).BuildImmutableTable()
 		if len(tblDesc.PublicNonPrimaryIndexes()) > 0 {
-			_, err := sql.TriggerInspectJob(
+			jobID, err := sql.TriggerInspectJob(
 				ctx,
 				fmt.Sprintf("import-validation-%s", tblDesc.GetName()),
 				p.ExecCfg(),
@@ -305,7 +338,15 @@ func (r *importResumer) Resume(ctx context.Context, execCtx interface{}) error {
 			if err != nil {
 				return errors.Wrapf(err, "failed to trigger inspect for import validation for table %s", tblDesc.GetName())
 			}
-			log.Eventf(ctx, "triggered inspect job for import validation for table %s with AOST %s", tblDesc.GetName(), setPublicTimestamp)
+			log.Eventf(ctx, "triggered inspect job %d for import validation for table %s with AOST %s", jobID, tblDesc.GetName(), setPublicTimestamp)
+
+			// For sync mode, wait for the inspect job to complete.
+			if validationMode == ImportRowCountValidationSync {
+				if err := p.ExecCfg().JobRegistry.WaitForJobs(ctx, []jobspb.JobID{jobID}); err != nil {
+					return errors.Wrapf(err, "failed to wait for inspect job %d for table %s", jobID, tblDesc.GetName())
+				}
+				log.Eventf(ctx, "inspect job %d completed for table %s", jobID, tblDesc.GetName())
+			}
 		}
 	}
 

pkg/sql/inspect/index_consistency_check.go

@@ -379,12 +379,26 @@ func (c *indexConsistencyCheck) loadCatalogInfo(ctx context.Context) error {
 			// We can only check a secondary index that has a 1-to-1 mapping between
 			// keys in the primary index. Unsupported indexes should be filtered out
 			// when the job is created.
+			// TODO(154862): support partial indexes
 			if idx.IsPartial() {
 				return errors.AssertionFailedf(
 					"unsupported index type for consistency check: partial index",
 				)
 			}
+			// TODO(154762): support hash sharded indexes
+			if idx.IsSharded() {
+				return errors.AssertionFailedf(
+					"unsupported index type for consistency check: hash-sharded index",
+				)
+			}
+			// TODO(154772): support expression indexes
+			if c.tableDesc.IsExpressionIndex(idx) {
+				return errors.AssertionFailedf(
+					"unsupported index type for consistency check: expression index",
+				)
+			}
 			switch idx.GetType() {
+			// TODO(154860): support inverted indexes
 			case idxtype.INVERTED, idxtype.VECTOR:
 				return errors.AssertionFailedf(
 					"unsupported index type for consistency check: %s", idx.GetType(),

pkg/sql/logictest/testdata/logic_test/inspect

@@ -137,3 +137,43 @@ statement ok
 INSPECT DATABASE test;
 
 subtest end
+
+subtest inspect_unsupported_indexes
+
+user root
+
+# Test that unsupported indexes (hash-sharded, expression) are skipped.
+# Table has both unsupported indexes and a regular index.
+statement ok
+CREATE TABLE t2 (x INT, y INT, z INT, INDEX hash_idx (x) USING HASH, INDEX expr_idx ((y + z)), INDEX regular_idx (z));
+
+statement ok
+INSERT INTO t2 (x, y, z) VALUES (1, 1, 1), (2, 2, 2), (3, 3, 3);
+
+# Should succeed, checking only the regular index.
+skipif config local-mixed-25.2 local-mixed-25.3
+statement ok
+EXPERIMENTAL SCRUB TABLE t2 AS OF SYSTEM TIME '-1us';
+
+# Verify only one check was created (for the regular index).
+skipif config local-mixed-25.2 local-mixed-25.3
+query TI
+SELECT
+  json_extract_path_text(
+    crdb_internal.pb_to_json('cockroach.sql.jobs.jobspb.Payload', payload),
+    'inspectDetails', 'checks', '0', 'type'
+  ) AS check_type,
+  jsonb_array_length(
+    crdb_internal.pb_to_json('cockroach.sql.jobs.jobspb.Payload', payload) -> 'inspectDetails' -> 'checks'
+  ) AS num_checks
+FROM crdb_internal.system_jobs
+WHERE job_type = 'INSPECT'
+ORDER BY created DESC
+LIMIT 1
+----
+INSPECT_CHECK_INDEX_CONSISTENCY  1
+
+statement ok
+DROP TABLE t2;
+
+subtest end

pkg/sql/opt/exec/execbuilder/testdata/explain_gist

@@ -288,3 +288,21 @@ SELECT crdb_internal.decode_plan_gist('AgHk+v//3xoEAKAFAgAABQQGBA==');
 • virtual table
   table: @?
   spans: 1+ spans
+
+# Regression tests for #154300. Gracefully handle invalid gists.
+query T nosort
+SELECT crdb_internal.decode_external_plan_gist('AgYd':::STRING);
+----
+
+query T nosort
+SELECT crdb_internal.decode_external_plan_gist('Agwc':::STRING);
+----
+• root
+│
+├── • explain
+│
+└── • subquery
+    │ id: @S1
+    │ exec mode: exists
+    │
+    └── • group (scalar)