Merge cockroachdb#106626 cockroachdb#106635

106626: sctest: Fix bug that backup/restore suite would secretely early return r=Xiang-Gu a=Xiang-Gu

   Previously, the backup/restore testing suite would quietly early return
    on certain test cases without running any actual content (i.e. no
    backups were taken nor restores were done). This commit fixes that.

  Fix cockroachdb#104205

   Release note: None

106635: compose: Deflake TestComposeCompare r=Xiang-Gu a=Xiang-Gu

We identified and fixed two issues in TestComposeCompare failure as reported by TeamCity:
 1. Certain collations used in the test are supported in CRDB but not in Postgres. We changed them to be such that both systems support.
 2. CRDB does not support ALTER TABLE INJECT STATISTICS within an explicit transaction and we disabled them in the test.

Informs cockroachdb#82867

Release note: None

Co-authored-by: Xiang Gu <[email protected]>

pkg/ccl/backupccl/backupresolver/targets.go

@@ -280,24 +280,28 @@ func NewDescriptorResolver(descs []catalog.Descriptor) (*DescriptorResolver, err
 			scName = scDesc.GetName()
 		}
 
-		// Create an entry for the descriptor.
-		objMap := schemaMap[scName]
-		if objMap == nil {
-			objMap = make(map[string]descpb.ID)
-		}
-		descName := desc.GetName()
-		// Handle special case of system.namespace table which used to be named
-		// system.namespace2.
-		if desc.GetID() == keys.NamespaceTableID &&
-			desc.GetPostDeserializationChanges().Contains(catalog.UpgradedNamespaceName) {
-			descName = catconstants.PreMigrationNamespaceTableName
-		}
-		if _, ok := objMap[descName]; ok {
-			return errors.Errorf("duplicate %s name: %q.%q.%q used for ID %d and %d",
-				kind, parentDesc.GetName(), scName, descName, desc.GetID(), objMap[descName])
-		}
-		objMap[descName] = desc.GetID()
-		r.ObjsByName[parentDesc.GetID()][scName] = objMap
+		// Create an entry for the descriptor in `r.ObjsByName` (if it's not a
+		// function) and `r.ObjIDsBySchema`.
+		// Note: `r.DescByID` has been previously populated already.
+		if kind != "function" {
+			objMap := schemaMap[scName]
+			if objMap == nil {
+				objMap = make(map[string]descpb.ID)
+			}
+			descName := desc.GetName()
+			// Handle special case of system.namespace table which used to be named
+			// system.namespace2.
+			if desc.GetID() == keys.NamespaceTableID &&
+				desc.GetPostDeserializationChanges().Contains(catalog.UpgradedNamespaceName) {
+				descName = catconstants.PreMigrationNamespaceTableName
+			}
+			if _, ok := objMap[descName]; ok {
+				return errors.Errorf("duplicate %s name: %q.%q.%q used for ID %d and %d",
+					kind, parentDesc.GetName(), scName, descName, desc.GetID(), objMap[descName])
+			}
+			objMap[descName] = desc.GetID()
+			r.ObjsByName[parentDesc.GetID()][scName] = objMap
+		}
 
 		objIDsMap := r.ObjIDsBySchema[parentDesc.GetID()]
 		objIDs := objIDsMap[scName]

pkg/compose/compare/compare/compare_test.go

@@ -109,10 +109,8 @@ func TestCompare(t *testing.T) {
 				{
 					name: "cockroach2",
 					mutators: []randgen.Mutator{
-						randgen.StatisticsMutator,
 						randgen.ForeignKeyMutator,
 						randgen.ColumnFamilyMutator,
-						randgen.StatisticsMutator,
 						randgen.IndexStoringMutator,
 						randgen.PartialIndexMutator,
 					},

pkg/sql/randgen/type.go

@@ -34,7 +34,7 @@ var (
 	// arrayContentsTypes contains all of the types that are valid to store within
 	// an array.
 	arrayContentsTypes []*types.T
-	collationLocales   = [...]string{"da", "de", "en"}
+	collationLocales   = [...]string{"da_DK", "de_DE", "en_US"}
 )
 
 func init() {

pkg/sql/schemachanger/sctest/cumulative.go

@@ -1091,7 +1091,7 @@ func Backup(t *testing.T, path string, newCluster NewClusterFunc) {
 		}
 		successExpected := atomic.Bool{}
 		stageChan := make(chan stage)
-		var closedStageChan bool // mark when stageChan is closed in the callback
+		var closeStageChan sync.Once
 		ctx, cancel := context.WithCancel(context.Background())
 		_, db, cleanup := newCluster(t, &scexec.TestingKnobs{
 			BeforeStage: func(p scplan.Plan, stageIdx int) error {
@@ -1100,23 +1100,20 @@ func Backup(t *testing.T, path string, newCluster NewClusterFunc) {
 				if p.Stages[stageIdx].Type() == scop.BackfillType && hasDMLInSetup {
 					successExpected.Store(false)
 				}
-				// If the plan has no post-commit stages, we'll close the
-				// stageChan eagerly.
-				if p.Stages[len(p.Stages)-1].Phase < scop.PostCommitPhase {
-
-					// The other test goroutine will set stageChan to nil later on.
-					// We only want to close it once, and then we don't want to look
-					// at it again. Avoid the racy access to stageChan by consulting
-					// the bool.
-					if !closedStageChan {
-						closedStageChan = true
-						close(stageChan)
-					}
+				if p.Stages[stageIdx].Phase == scop.StatementPhase {
 					return nil
 				}
-				if p.Stages[stageIdx].Phase < scop.PostCommitPhase {
+				if p.Stages[stageIdx].Phase == scop.PreCommitPhase {
+					if p.Stages[len(p.Stages)-1].Phase < scop.PostCommitPhase {
+						// We've seen all stmts in the test case (bc we're in PreCommitPhase)
+						// and there is no PostCommitPhase in the plan.
+						closeStageChan.Do(func() {
+							close(stageChan)
+						})
+					}
 					return nil
 				}
+
 				if stageChan != nil {
 					s := stage{p: p, stageIdx: stageIdx, resume: make(chan error)}
 					select {