Merge #117793

117793: sql/schemachanger: enable declarative CREATE SEQUENCE r=fqazi a=fqazi

Previously, we did fully implement CREATE SEQUENCE in the declarative schema changer but had it disabled by default since we were missing this PR will do the following:

- Enable CREATE SEQUENCE by default in the declarative schema changer
- Add tests for transactionally creating / dropping sequences to confirm that the rules are sound
- Address bugs with transactional creation / drops of relations, since we previously did not create data elements
- Add tests for transactionally adding and using a sequence as a column
- Skip back up and restore tests for newly created objects, since intermediate states are not backed up today (if we do CREATE SEQUENCE and ADD COLUMN in a single txn, we will intentionally not backup the sequence during the job phase)


Informs: #117648
Epic: [CRDB-31282](https://cockroachlabs.atlassian.net/browse/CRDB-31282)

Co-authored-by: Faizan Qazi <[email protected]>

pkg/sql/catalog/rewrite/rewrite.go

@@ -748,7 +748,7 @@ func rewriteSchemaChangerState(
 		t := &state.Targets[i]
 		// Since the parent database ID is never written in the descriptorRewrites
 		// map we need to special case certain elements that need their ParentID
-		// re-written.
+		// re-written
 		if data := t.GetTableData(); data != nil {
 			rewrite, ok := descriptorRewrites[data.TableID]
 			if !ok {
@@ -757,6 +757,15 @@ func rewriteSchemaChangerState(
 			data.TableID = rewrite.ID
 			data.DatabaseID = rewrite.ParentID
 			continue
+		} else if data := t.GetNamespace(); data != nil {
+			rewrite, ok := descriptorRewrites[data.DescriptorID]
+			if !ok {
+				return errors.Errorf("missing rewrite for id %d in %s", data.DescriptorID, screl.ElementString(t.Element()))
+			}
+			data.DescriptorID = rewrite.ID
+			data.DatabaseID = rewrite.ParentID
+			data.SchemaID = rewrite.ParentSchemaID
+			continue
 		}
 		if err := screl.WalkDescIDs(t.Element(), func(id *descpb.ID) error {
 			if *id == descpb.InvalidID {

pkg/sql/logictest/testdata/logic_test/new_schema_changer

@@ -1523,16 +1523,16 @@ EXPLAIN (DDL) ALTER TABLE stmt_ctrl ADD COLUMN j BOOL
 statement ok
 ALTER TABLE stmt_ctrl ADD COLUMN fallback_works BOOL
 
-# Validate that CREATE SEQUENCE is disabled and can be re-enabled with the same flag.
+# Validate that CREATE SCHEMA is disabled and can be re-enabled with the same flag.
 statement error pgcode 0A000 cannot explain a statement which is not supported by the declarative schema changer
-EXPLAIN (DDL) CREATE SEQUENCE sq2
+EXPLAIN (DDL) CREATE SCHEMA schema1
 
 statement ok
-SET CLUSTER SETTING sql.schema.force_declarative_statements='+CREATE SEQUENCE'
+SET CLUSTER SETTING sql.schema.force_declarative_statements='+CREATE SCHEMA'
 
 skipif config local-mixed-23.1
 statement ok
-EXPLAIN (DDL) CREATE SEQUENCE sq2
+EXPLAIN (DDL) CREATE SCHEMA sc1
 
 subtest end
 

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_sequence.go

@@ -109,6 +109,18 @@ func CreateSequence(b BuildCtx, n *tree.CreateSequence) {
 		Name:         string(n.Name.ObjectName),
 	}
 	b.Add(sequenceNamespace)
+	// Set up a schema child entry. This will be a no-op for relations.
+	sequenceSchemaChild := &scpb.SchemaChild{
+		ChildObjectID: sequenceID,
+		SchemaID:      schemaElem.SchemaID,
+	}
+	b.Add(sequenceSchemaChild)
+	// Add a table data element, this go public with the descriptor.
+	tableData := &scpb.TableData{
+		TableID:    sequenceID,
+		DatabaseID: dbElem.DatabaseID,
+	}
+	b.Add(tableData)
 	// Add any sequence options.
 	options := scdecomp.GetSequenceOptions(sequenceElem.SequenceID, &tempSequenceOpts)
 	for _, opt := range options {

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/process.go

@@ -65,7 +65,7 @@ var supportedStatements = map[reflect.Type]supportedStatement{
 	reflect.TypeOf((*tree.DropRoutine)(nil)):         {fn: DropFunction, statementTags: []string{tree.DropFunctionTag, tree.DropProcedureTag}, on: true, checks: nil},
 	reflect.TypeOf((*tree.CreateRoutine)(nil)):       {fn: CreateFunction, statementTags: []string{tree.CreateFunctionTag, tree.CreateProcedureTag}, on: true, checks: nil},
 	reflect.TypeOf((*tree.CreateSchema)(nil)):        {fn: CreateSchema, statementTags: []string{tree.CreateSchemaTag}, on: false, checks: isV232Active},
-	reflect.TypeOf((*tree.CreateSequence)(nil)):      {fn: CreateSequence, statementTags: []string{tree.CreateSequenceTag}, on: false, checks: isV232Active},
+	reflect.TypeOf((*tree.CreateSequence)(nil)):      {fn: CreateSequence, statementTags: []string{tree.CreateSequenceTag}, on: true, checks: isV241Active},
 	reflect.TypeOf((*tree.CreateDatabase)(nil)):      {fn: CreateDatabase, statementTags: []string{tree.CreateDatabaseTag}, on: true, checks: isV241Active},
 }
 

pkg/sql/schemachanger/scbuild/testdata/create_sequence

@@ -10,6 +10,10 @@ CREATE SEQUENCE db.public.sq1 MINVALUE 1 MAXVALUE 9223372036854775807 INCREMENT
   {sequenceId: 107}
 - [[Namespace:{DescID: 107, Name: sq1, ReferencedDescID: 104}, PUBLIC], ABSENT]
   {databaseId: 104, descriptorId: 107, name: sq1, schemaId: 105}
+- [[SchemaChild:{DescID: 107, ReferencedDescID: 105}, PUBLIC], ABSENT]
+  {childObjectId: 107, schemaId: 105}
+- [[TableData:{DescID: 107, ReferencedDescID: 104}, PUBLIC], ABSENT]
+  {databaseId: 104, tableId: 107}
 - [[SequenceOption:{DescID: 107, Name: START}, PUBLIC], ABSENT]
   {key: START, sequenceId: 107, value: "32"}
 - [[IndexData:{DescID: 106, IndexID: 1}, PUBLIC], PUBLIC]

pkg/sql/schemachanger/scplan/internal/opgen/op_funcs.go

@@ -51,6 +51,7 @@ func statementForDropJob(e scpb.Element, md *opGenContext) scop.StatementForDrop
 type opGenContext struct {
 	scpb.TargetState
 	Current         []scpb.Status
+	Initial         []scpb.Status
 	ActiveVersion   clusterversion.ClusterVersion
 	elementToTarget map[scpb.Element]int
 	InRollback      bool
@@ -63,6 +64,7 @@ func makeOpgenContext(
 		ActiveVersion:   activeVersion,
 		InRollback:      cs.InRollback,
 		TargetState:     cs.TargetState,
+		Initial:         cs.Initial,
 		Current:         cs.Current,
 		elementToTarget: make(map[scpb.Element]int),
 	}
@@ -187,8 +189,8 @@ func checkIfDescriptorIsWithoutData(id descpb.ID, md *opGenContext) bool {
 		case *scpb.IndexData, *scpb.TableData:
 			// Check if this descriptor has any data within
 			// a public state.
-			if md.Current[idx] == scpb.Status_PUBLIC ||
-				t.TargetStatus == scpb.Status_PUBLIC {
+			if md.Current[idx] == scpb.Status_PUBLIC &&
+				md.Initial[idx] == scpb.Status_PUBLIC {
 				doesDescriptorHaveData = true
 			}
 		}

pkg/sql/schemachanger/scplan/internal/opgen/opgen_column.go

@@ -43,7 +43,11 @@ func init() {
 						ColumnID: this.ColumnID,
 					}
 				}),
-				emit(func(this *scpb.Column) *scop.RefreshStats {
+				emit(func(this *scpb.Column, md *opGenContext) *scop.RefreshStats {
+					// No need to generate stats for empty descriptors.
+					if checkIfDescriptorIsWithoutData(this.TableID, md) {
+						return nil
+					}
 					return &scop.RefreshStats{
 						TableID: this.TableID,
 					}

pkg/sql/schemachanger/scplan/internal/rules/current/dep_create.go

@@ -38,7 +38,7 @@ func init() {
 		"dependent", "relation",
 		func(from, to NodeVars) rel.Clauses {
 			return rel.Clauses{
-				from.TypeFilter(rulesVersionKey, Not(isDescriptor)),
+				from.TypeFilter(rulesVersionKey, Not(isDescriptor), Not(isData)),
 				to.TypeFilter(rulesVersionKey, isDescriptor),
 				JoinOnDescID(from, to, "relation-id"),
 				StatusesToPublicOrTransient(from, scpb.Status_PUBLIC, to, scpb.Status_PUBLIC),

pkg/sql/schemachanger/scplan/internal/rules/current/dep_garbage_collection.go

@@ -138,3 +138,21 @@ func init() {
 		},
 	)
 }
+
+// Rules to ensure for created objects the table data will be live after the
+// descriptor is public.
+func init() {
+	registerDepRule(
+		"table added right before data element",
+		scgraph.Precedence,
+		"table", "data",
+		func(from, to NodeVars) rel.Clauses {
+			return rel.Clauses{
+				from.TypeFilter(rulesVersionKey, isDescriptor),
+				to.TypeFilter(rulesVersionKey, isData),
+				JoinOnDescID(from, to, "table-id"),
+				StatusesToPublicOrTransient(from, scpb.Status_PUBLIC, to, scpb.Status_PUBLIC),
+			}
+		},
+	)
+}

pkg/sql/schemachanger/scplan/internal/rules/current/dep_two_version.go

@@ -72,10 +72,12 @@ func init() {
 			from.Node.AttrEq(screl.CurrentStatus, t.From()),
 			to.Node.AttrEq(screl.CurrentStatus, t.To()),
 			descriptorIsNotBeingDropped(from.El),
-			// Make sure to join a data element o confirm that data exists.
+			// Make sure to join a data element to confirm that data exists.
 			descriptorData.Type((*scpb.TableData)(nil)),
-			descriptorData.JoinTarget(),
+			descriptorData.JoinTargetNode(),
+			descriptorData.CurrentStatus(scpb.Status_PUBLIC),
 			descriptorData.DescIDEq(descID),
+			descriptorDataIsNotBeingAdded(descID),
 		}
 		if len(prePrevStatuses) > 0 {
 			clauses = append(clauses,

pkg/sql/schemachanger/scplan/internal/rules/current/helpers.go

@@ -44,6 +44,30 @@ var descriptorIsNotBeingDropped = screl.Schema.DefNotJoin1(
 	},
 )
 
+// descriptorDataIsNotBeingAdded indicates if we are operating on a descriptor
+// that already exists and was not created in the current transaction. This is
+// determined by detecting if the data element is public, and not going from
+// absent to public which newly created descriptors will.
+var descriptorDataIsNotBeingAdded = screl.Schema.DefNotJoin1(
+	"descriptorIsDataNotBeingAdded"+rulesVersion, "descID", func(
+		descID rel.Var,
+	) rel.Clauses {
+		descriptorData := rules.MkNodeVars("descriptor-data")
+		prevDescriptorData := rules.MkNodeVars("prev-descriptor-data")
+		return rel.Clauses{
+			descriptorData.Type((*scpb.TableData)(nil)),
+			descriptorData.JoinTargetNode(),
+			descriptorData.CurrentStatus(scpb.Status_PUBLIC),
+			descriptorData.DescIDEq(descID),
+			prevDescriptorData.Type((*scpb.TableData)(nil)),
+			prevDescriptorData.JoinTargetNode(),
+			prevDescriptorData.CurrentStatus(scpb.Status_ABSENT),
+			prevDescriptorData.DescIDEq(descID),
+			prevDescriptorData.El.AttrEqVar(rel.Self, descriptorData.El),
+		}
+	},
+)
+
 // isDescriptor returns true for a descriptor-element, i.e. an element which
 // owns its corresponding descriptor.
 func isDescriptor(e scpb.Element) bool {