Merge #153599 #153814

153599: sql: fix incorrect min timestamp error check r=mgartner a=mgartner

Release note: None

153814: sql: set always_distribute_full_scans for internal validation queries r=rafiss a=rafiss

The validation queries we run after a backfill all use full table scans, and usually there are no stats available. Since dd57482 was merged, we need to explicitly isntruct the optimizer to distribute these full scans.

fixes #152859
Release note: None

Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>

Author: 3 people

pkg/cmd/roachtest/tests/schemachange.go

@@ -433,13 +433,6 @@ func makeSchemaChangeBulkIngestTest(
 				db := c.Conn(ctx, t.L(), 1)
 				defer db.Close()
 
-				// TODO(152859): Temporarily disable soft limit for distribute scan to
-				// test performance regression fix (see #152295).
-				t.L().Printf("Setting use_soft_limit_for_distribute_scan = false")
-				if _, err := db.Exec("SET use_soft_limit_for_distribute_scan = false"); err != nil {
-					t.Fatal(err)
-				}
-
 				t.L().Printf("Computing table statistics manually")
 				if _, err := db.Exec("CREATE STATISTICS stats from bulkingest.bulkingest"); err != nil {
 					t.Fatal(err)

pkg/sql/backfill.go

@@ -1541,6 +1541,10 @@ func ValidateConstraint(
 	runHistoricalTxn descs.HistoricalInternalExecTxnRunner,
 	execOverride sessiondata.InternalExecutorOverride,
 ) (err error) {
+	// Validation queries use full table scans which we always want to distribute.
+	// See https://github.com/cockroachdb/cockroach/issues/152859.
+	execOverride.AlwaysDistributeFullScans = true
+
 	tableDesc, err = tableDesc.MakeFirstMutationPublic(catalog.IgnoreConstraints)
 	if err != nil {
 		return err
@@ -1666,6 +1670,10 @@ func ValidateInvertedIndexes(
 	execOverride sessiondata.InternalExecutorOverride,
 	protectedTSManager scexec.ProtectedTimestampManager,
 ) (err error) {
+	// Validation queries use full table scans which we always want to distribute.
+	// See https://github.com/cockroachdb/cockroach/issues/152859.
+	execOverride.AlwaysDistributeFullScans = true
+
 	grp := ctxgroup.WithContext(ctx)
 	invalid := make(chan descpb.IndexID, len(indexes))
 
@@ -1769,6 +1777,11 @@ func countExpectedRowsForInvertedIndex(
 ) (int64, error) {
 	desc := tableDesc
 	start := timeutil.Now()
+
+	// Validation queries use full table scans which we always want to distribute.
+	// See https://github.com/cockroachdb/cockroach/issues/152859.
+	execOverride.AlwaysDistributeFullScans = true
+
 	if withFirstMutationPublic {
 		// Make the mutations public in an in-memory copy of the descriptor and
 		// add it to the Collection's synthetic descriptors, so that we can use
@@ -1867,6 +1880,10 @@ func ValidateForwardIndexes(
 	execOverride sessiondata.InternalExecutorOverride,
 	protectedTSManager scexec.ProtectedTimestampManager,
 ) (err error) {
+	// Validation queries use full table scans which we always want to distribute.
+	// See https://github.com/cockroachdb/cockroach/issues/152859.
+	execOverride.AlwaysDistributeFullScans = true
+
 	grp := ctxgroup.WithContext(ctx)
 
 	invalid := make(chan descpb.IndexID, len(indexes))
@@ -1983,6 +2000,10 @@ func populateExpectedCounts(
 	runHistoricalTxn descs.HistoricalInternalExecTxnRunner,
 	execOverride sessiondata.InternalExecutorOverride,
 ) (int64, error) {
+	// Validation queries use full table scans which we always want to distribute.
+	// See https://github.com/cockroachdb/cockroach/issues/152859.
+	execOverride.AlwaysDistributeFullScans = true
+
 	desc := tableDesc
 	if withFirstMutationPublic {
 		// The query to count the expected number of rows can reference columns
@@ -2049,6 +2070,10 @@ func countIndexRowsAndMaybeCheckUniqueness(
 	runHistoricalTxn descs.HistoricalInternalExecTxnRunner,
 	execOverride sessiondata.InternalExecutorOverride,
 ) (int64, error) {
+	// Validation queries use full table scans which we always want to distribute.
+	// See https://github.com/cockroachdb/cockroach/issues/152859.
+	execOverride.AlwaysDistributeFullScans = true
+
 	// If we are doing a REGIONAL BY ROW locality change, we can
 	// bypass the uniqueness check below as we are only adding or
 	// removing an implicit partitioning column.  Scan the

pkg/sql/check.go

@@ -63,11 +63,17 @@ func validateCheckExpr(
 		queryStr = fmt.Sprintf(`SELECT %s FROM [%d AS t]@[%d] WHERE NOT (%s) LIMIT 1`, columns, tableDesc.GetID(), indexIDForValidation, exprStr)
 	}
 	log.Dev.Infof(ctx, "validating check constraint %q with query %q", formattedCkExpr, queryStr)
+
+	// Validation queries use full table scans which we always want to distribute.
+	// See https://github.com/cockroachdb/cockroach/issues/152859.
+	execOverride := sessiondata.NodeUserSessionDataOverride
+	execOverride.AlwaysDistributeFullScans = true
+
 	violatingRow, err = txn.QueryRowEx(
 		ctx,
 		"validate check constraint",
 		txn.KV(),
-		sessiondata.NodeUserSessionDataOverride,
+		execOverride,
 		queryStr)
 	if err != nil {
 		return nil, formattedCkExpr, err

pkg/sql/conn_executor.go

@@ -3874,8 +3874,6 @@ func (ex *connExecutor) GetPCRReaderTimestamp() hlc.Timestamp {
 	return hlc.Timestamp{}
 }
 
-var minTSErr = kvpb.NewMinTimestampBoundUnsatisfiableError(hlc.Timestamp{}, hlc.Timestamp{})
-
 // resetEvalCtx initializes the fields of evalCtx that can change
 // during a session (i.e. the fields not set by initEvalCtx).
 //
@@ -3914,7 +3912,8 @@ func (ex *connExecutor) resetEvalCtx(evalCtx *extendedEvalContext, txn *kv.Txn,
 
 	// See resetPlanner for more context on setting the maximum timestamp for
 	// AOST read retries.
-	if err := ex.state.mu.autoRetryReason; err != nil && errors.Is(err, minTSErr) {
+	var minTSErr *kvpb.MinTimestampBoundUnsatisfiableError
+	if err := ex.state.mu.autoRetryReason; err != nil && errors.HasType(err, minTSErr) {
 		evalCtx.AsOfSystemTime.MaxTimestampBound = ex.extraTxnState.descCollection.GetMaxTimestampBound()
 	} else if newTxn {
 		evalCtx.AsOfSystemTime = nil

pkg/sql/internal.go

@@ -965,6 +965,9 @@ func applyOverrides(o sessiondata.InternalExecutorOverride, sd *sessiondata.Sess
 	if o.BufferedWritesEnabled != nil {
 		sd.BufferedWritesEnabled = *o.BufferedWritesEnabled
 	}
+	if o.AlwaysDistributeFullScans {
+		sd.AlwaysDistributeFullScans = true
+	}
 	// For 25.2, we're being conservative and explicitly disabling buffered
 	// writes for the internal executor.
 	// TODO(yuzefovich): remove this for 25.3.

pkg/sql/sessiondata/internal.go

@@ -87,6 +87,9 @@ type InternalExecutorOverride struct {
 	// BufferedWritesEnabled, if set, controls whether the buffered writes KV transaction
 	// protocol is used for user queries on the current session.
 	BufferedWritesEnabled *bool
+	// AlwaysDistributeFullScans, if true, overrides the
+	// always_distribute_full_scans session variable.
+	AlwaysDistributeFullScans bool
 }
 
 // NoSessionDataOverride is the empty InternalExecutorOverride which does not