Merge #63411 #63638 #63645 #63652

63411: opt: make check constraint and partial index columns anonymous r=mgartner a=mgartner

#### opt: make check constraint and partial index columns anonymous

This commit updates optbuilder so that synthesized columns used for
maintaining check constraints and partial indexes are anonymized in the
scope resulting from their projection. This prevents these columns from
being found in the scope when resolving columns while building
expressions. This simplifies some code and eliminates a class of bugs
that can cause "ambiguous column" errors when a user's table has column
names that match the synthesized column names. Now that these
synthesized columns are anonymous, there cannot be ambiguity between
them and table columns.

Release note: None

#### opt: simplify partial index PUT/DEL column building

Previously, the scopes to build synthesized partial index PUT and DEL
columns were passed as input the building functions. They were always
called with `mb.outScope` or `mb.fetchScope`, so there was no need to
provide them as arguments. The building functions no longer accept any
arguments.

Release note: None

63638: gcjob: share ClearRange logic for tables and indexes r=pbardea a=ajwerner

No obvious reason to keep these separate.

Release note: None

63645: scripts: allow GCEWORKER_USER env to override ssh user r=RaduBerinde a=cucaroach

fixes #63641


63652: kv/kvserver: skip TestClosedTimestampWorksWhenRequestsAreSentToNonLeaseHolders r=RaduBerinde a=RaduBerinde

Refs: #60682

Reason: flaky test

Generated by bin/skip-test.

Release justification: non-production code changes

Release note: None

Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: Tommy Reilly <[email protected]>
Co-authored-by: Radu Berinde <[email protected]>

pkg/kv/kvserver/client_closed_timestamp_test.go

@@ -20,6 +20,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/liveness/livenesspb"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
@@ -35,6 +36,7 @@ import (
 // becomes the leaseholder. See #48553 for more details.
 func TestClosedTimestampWorksWhenRequestsAreSentToNonLeaseHolders(t *testing.T) {
 	defer leaktest.AfterTest(t)()
+	skip.WithIssue(t, 60682, "flaky test")
 	defer log.Scope(t).Close(t)
 
 	ctx := context.Background()

pkg/sql/gcjob/index_garbage_collection.go

@@ -14,6 +14,7 @@ import (
 	"context"
 
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
+	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/sql"
@@ -116,17 +117,16 @@ func clearIndex(
 	}
 
 	sp := tableDesc.IndexSpan(execCfg.Codec, index.ID)
-
-	// ClearRange cannot be run in a transaction, so create a
-	// non-transactional batch to send the request.
-	b := &kv.Batch{}
-	b.AddRawRequest(&roachpb.ClearRangeRequest{
-		RequestHeader: roachpb.RequestHeader{
-			Key:    sp.Key,
-			EndKey: sp.EndKey,
-		},
-	})
-	return execCfg.DB.Run(ctx, b)
+	start, err := keys.Addr(sp.Key)
+	if err != nil {
+		return errors.Errorf("failed to addr index start: %v", err)
+	}
+	end, err := keys.Addr(sp.EndKey)
+	if err != nil {
+		return errors.Errorf("failed to addr index end: %v", err)
+	}
+	rSpan := roachpb.RSpan{Key: start, EndKey: end}
+	return clearSpanData(ctx, execCfg.DB, execCfg.DistSender, rSpan)
 }
 
 // completeDroppedIndexes updates the mutations of the table descriptor to

pkg/sql/gcjob/table_garbage_collection.go

@@ -101,6 +101,12 @@ func ClearTableData(
 
 	tableKey := roachpb.RKey(codec.TablePrefix(uint32(table.GetID())))
 	tableSpan := roachpb.RSpan{Key: tableKey, EndKey: tableKey.PrefixEnd()}
+	return clearSpanData(ctx, db, distSender, tableSpan)
+}
+
+func clearSpanData(
+	ctx context.Context, db *kv.DB, distSender *kvcoord.DistSender, span roachpb.RSpan,
+) error {
 
 	// ClearRange requests lays down RocksDB range deletion tombstones that have
 	// serious performance implications (#24029). The logic below attempts to
@@ -126,20 +132,20 @@ func ClearTableData(
 	const waitTime = 500 * time.Millisecond
 
 	var n int
-	lastKey := tableSpan.Key
+	lastKey := span.Key
 	ri := kvcoord.NewRangeIterator(distSender)
 	timer := timeutil.NewTimer()
 	defer timer.Stop()
 
-	for ri.Seek(ctx, tableSpan.Key, kvcoord.Ascending); ; ri.Next(ctx) {
+	for ri.Seek(ctx, span.Key, kvcoord.Ascending); ; ri.Next(ctx) {
 		if !ri.Valid() {
 			return ri.Error()
 		}
 
-		if n++; n >= batchSize || !ri.NeedAnother(tableSpan) {
+		if n++; n >= batchSize || !ri.NeedAnother(span) {
 			endKey := ri.Desc().EndKey
-			if tableSpan.EndKey.Less(endKey) {
-				endKey = tableSpan.EndKey
+			if span.EndKey.Less(endKey) {
+				endKey = span.EndKey
 			}
 			var b kv.Batch
 			b.AddRawRequest(&roachpb.ClearRangeRequest{
@@ -163,7 +169,7 @@ func ClearTableData(
 			}
 		}
 
-		if !ri.NeedAnother(tableSpan) {
+		if !ri.NeedAnother(span) {
 			break
 		}
 	}

pkg/sql/opt/optbuilder/delete.go

@@ -77,7 +77,7 @@ func (mb *mutationBuilder) buildDelete(returning tree.ReturningExprs) {
 	mb.buildFKChecksAndCascadesForDelete()
 
 	// Project partial index DEL boolean columns.
-	mb.projectPartialIndexDelCols(mb.fetchScope)
+	mb.projectPartialIndexDelCols()
 
 	private := mb.makeMutationPrivate(returning != nil)
 	mb.outScope.expr = mb.b.factory.ConstructDelete(

pkg/sql/opt/optbuilder/insert.go

@@ -652,16 +652,11 @@ func (mb *mutationBuilder) buildInsert(returning tree.ReturningExprs) {
 	// check constraint, refer to the correct columns.
 	mb.disambiguateColumns()
 
-	// Keep a reference to the scope before the check constraint columns are
-	// projected. We use this scope when projecting the partial index put
-	// columns because the check columns are not in-scope for those expressions.
-	preCheckScope := mb.outScope
-
 	// Add any check constraint boolean columns to the input.
 	mb.addCheckConstraintCols()
 
 	// Project partial index PUT boolean columns.
-	mb.projectPartialIndexPutCols(preCheckScope)
+	mb.projectPartialIndexPutCols()
 
 	mb.buildUniqueChecksForInsert()
 
@@ -869,11 +864,6 @@ func (mb *mutationBuilder) buildUpsert(returning tree.ReturningExprs) {
 	// check constraint, refer to the correct columns.
 	mb.disambiguateColumns()
 
-	// Keep a reference to the scope before the check constraint columns are
-	// projected. We use this scope when projecting the partial index put
-	// columns because the check columns are not in-scope for those expressions.
-	preCheckScope := mb.outScope
-
 	// Add any check constraint boolean columns to the input.
 	mb.addCheckConstraintCols()
 
@@ -890,9 +880,9 @@ func (mb *mutationBuilder) buildUpsert(returning tree.ReturningExprs) {
 	// mb.fetchScope will be nil. Therefore, we only project partial index
 	// PUT columns.
 	if mb.needExistingRows() {
-		mb.projectPartialIndexPutAndDelCols(preCheckScope, mb.fetchScope)
+		mb.projectPartialIndexPutAndDelCols()
 	} else {
-		mb.projectPartialIndexPutCols(preCheckScope)
+		mb.projectPartialIndexPutCols()
 	}
 
 	mb.buildUniqueChecksForUpsert()

pkg/sql/opt/optbuilder/mutation_builder.go

@@ -837,8 +837,8 @@ func findRoundingFunction(
 }
 
 // addCheckConstraintCols synthesizes a boolean output column for each check
-// constraint defined on the target table. The mutation operator will report
-// a constraint violation error if the value of the column is false.
+// constraint defined on the target table. The mutation operator will report a
+// constraint violation error if the value of the column is false.
 func (mb *mutationBuilder) addCheckConstraintCols() {
 	if mb.tab.CheckCount() != 0 {
 		projectionsScope := mb.outScope.replace()
@@ -860,6 +860,9 @@ func (mb *mutationBuilder) addCheckConstraintCols() {
 			referencedCols := &opt.ColSet{}
 			mb.b.buildScalar(texpr, mb.outScope, projectionsScope, scopeCol, referencedCols)
 
+			// Clear the column name so that it cannot be referenced.
+			scopeCol.clearName()
+
 			// Synthesized check columns are only necessary if the columns
 			// referenced in the check expression are being mutated. If they are
 			// not being mutated, we do not add the newly built column to
@@ -893,47 +896,23 @@ func (mb *mutationBuilder) mutationColumnIDs() opt.ColSet {
 // projectPartialIndexPutCols builds a Project that synthesizes boolean PUT
 // columns for each partial index defined on the target table. See
 // partialIndexPutColIDs for more info on these columns.
-//
-// putScope must contain the columns representing the values of each mutated row
-// AFTER the mutation is applied.
-func (mb *mutationBuilder) projectPartialIndexPutCols(putScope *scope) {
-	if putScope == nil {
-		panic(errors.AssertionFailedf("cannot project partial index PUT columns with nil scope"))
-	}
-	mb.projectPartialIndexColsImpl(putScope, nil /* delScope */)
+func (mb *mutationBuilder) projectPartialIndexPutCols() {
+	mb.projectPartialIndexColsImpl(mb.outScope, nil /* delScope */)
 }
 
 // projectPartialIndexDelCols builds a Project that synthesizes boolean PUT
 // columns for each partial index defined on the target table. See
 // partialIndexDelColIDs for more info on these columns.
-//
-// delScope must contain the columns representing the values of each mutated row
-// BEFORE the mutation is applied.
-func (mb *mutationBuilder) projectPartialIndexDelCols(delScope *scope) {
-	if delScope == nil {
-		panic(errors.AssertionFailedf("cannot project partial index DEL columns with nil scope"))
-	}
-	mb.projectPartialIndexColsImpl(nil /* putScope */, delScope)
+func (mb *mutationBuilder) projectPartialIndexDelCols() {
+	mb.projectPartialIndexColsImpl(nil /* putScope */, mb.fetchScope)
 }
 
-// projectPartialIndexPutAndDelCols builds a Project that synthesizes boolean PUT and
-// DEL columns for each partial index defined on the target table. See
+// projectPartialIndexPutAndDelCols builds a Project that synthesizes boolean
+// PUT and DEL columns for each partial index defined on the target table. See
 // partialIndexPutColIDs and partialIndexDelColIDs for more info on these
 // columns.
-//
-// putScope must contain the columns representing the values of each mutated row
-// AFTER the mutation is applied.
-//
-// delScope must contain the columns representing the values of each mutated row
-// BEFORE the mutation is applied.
-func (mb *mutationBuilder) projectPartialIndexPutAndDelCols(putScope, delScope *scope) {
-	if putScope == nil {
-		panic(errors.AssertionFailedf("cannot project partial index PUT columns with nil scope"))
-	}
-	if delScope == nil {
-		panic(errors.AssertionFailedf("cannot project partial index DEL columns with nil scope"))
-	}
-	mb.projectPartialIndexColsImpl(putScope, delScope)
+func (mb *mutationBuilder) projectPartialIndexPutAndDelCols() {
+	mb.projectPartialIndexColsImpl(mb.outScope, mb.fetchScope)
 }
 
 // projectPartialIndexColsImpl builds a Project that synthesizes boolean PUT and
@@ -967,6 +946,9 @@ func (mb *mutationBuilder) projectPartialIndexColsImpl(putScope, delScope *scope
 
 				mb.b.buildScalar(texpr, putScope, projectionScope, scopeCol, nil)
 				mb.partialIndexPutColIDs[ord] = scopeCol.id
+
+				// Clear the column name so that it cannot be referenced.
+				scopeCol.clearName()
 			}
 
 			// Build synthesized DEL columns.
@@ -977,6 +959,9 @@ func (mb *mutationBuilder) projectPartialIndexColsImpl(putScope, delScope *scope
 
 				mb.b.buildScalar(texpr, delScope, projectionScope, scopeCol, nil)
 				mb.partialIndexDelColIDs[ord] = scopeCol.id
+
+				// Clear the column name so that it cannot be referenced.
+				scopeCol.clearName()
 			}
 
 			ord++