scbuild: fix concurrent schema change verification bug

pkg/sql/schemachanger/scbuild/build.go

@@ -284,7 +284,7 @@ func newBuilderState(
 		panic(err)
 	}
 	for _, t := range incumbent.TargetState.Targets {
-		bs.ensureDescriptor(screl.GetDescID(t.Element()))
+		bs.ensureDescriptors(t.Element())
 	}
 	for i, t := range incumbent.TargetState.Targets {
 		bs.upsertElementState(elementState{

pkg/sql/schemachanger/scbuild/builder_state.go

@@ -68,12 +68,15 @@ func (b *builderState) Ensure(e scpb.Element, target scpb.TargetStatus, meta scp
 	default:
 		panic(errors.AssertionFailedf("unsupported target %s", target.Status()))
 	}
+
 	if dst == nil {
 		// We're adding both a new element and a target for it.
 		if target == scpb.ToAbsent {
 			// Ignore targets to remove something that doesn't exist yet.
 			return
 		}
+		// Set a target for the element but check for concurrent schema changes.
+		_ = b.checkForConcurrentSchemaChanges(e)
 		b.addNewElementState(elementState{
 			element:  e,
 			initial:  scpb.Status_ABSENT,
@@ -89,18 +92,10 @@ func (b *builderState) Ensure(e scpb.Element, target scpb.TargetStatus, meta scp
 		return
 	}
 
-	// Check that the descriptors relevant to this element are not undergoing any
-	// concurrent schema changes.
-	screl.AllTargetDescIDs(e).ForEach(func(id descpb.ID) {
-		b.ensureDescriptor(id)
-		if c := b.descCache[id]; c != nil && c.desc != nil && c.desc.HasConcurrentSchemaChanges() {
-			panic(scerrors.ConcurrentSchemaChangeError(c.desc))
-		}
-	})
-	// Re-assign dst because the above function may have mutated the builder
-	// state. Specifically, the output slice, to which dst points to, might
-	// have grown and might have been reallocated.
-	dst = b.getExistingElementState(e)
+	// Check that there are no concurrent schema changes on the descriptors
+	// referenced by this element. Re-assign dst because of potential
+	// re-allocations.
+	dst = b.checkForConcurrentSchemaChanges(e)
 
 	// We were about to overwrite an element's target and metadata. Assert one
 	// disallowed case: reviving a "ghost" element, that is, add an element that
@@ -182,6 +177,32 @@ func (b *builderState) Ensure(e scpb.Element, target scpb.TargetStatus, meta scp
 	panic(errors.AssertionFailedf("unsupported incumbent target %s", oldTarget.Status()))
 }
 
+func (b *builderState) checkForConcurrentSchemaChanges(e scpb.Element) *elementState {
+	b.ensureDescriptors(e)
+	// Check that there are no descriptors which are undergoing a concurrent
+	// schema change which might interfere with this one.
+	screl.AllTargetDescIDs(e).ForEach(func(id descpb.ID) {
+		if c := b.descCache[id]; c != nil && c.desc != nil && c.desc.HasConcurrentSchemaChanges() {
+			panic(scerrors.ConcurrentSchemaChangeError(c.desc))
+		}
+	})
+	// We may have mutated the builder state for this element.
+	// Specifically, the output slice might have grown and have been realloc'ed.
+	return b.getExistingElementState(e)
+}
+
+// ensureDescriptors ensures the presence of all elements for all
+// descriptors referenced inside the element.
+//
+// This provides us with all of the ID -> name mappings required to
+// comprehensively decorate any EXPLAIN (DDL) output.
+func (b *builderState) ensureDescriptors(e scpb.Element) {
+	_ = screl.WalkDescIDs(e, func(id *catid.DescID) error {
+		b.ensureDescriptor(*id)
+		return nil
+	})
+}
+
 func (b *builderState) upsertElementState(es elementState) {
 	if existing := b.getExistingElementState(es.element); existing != nil {
 		if err := b.localMemAcc.Grow(b.ctx, es.byteSize()-existing.byteSize()); err != nil {

pkg/sql/schemachanger/testdata/end_to_end/alter_table_add_check_udf/alter_table_add_check_udf.side_effects

@@ -162,7 +162,9 @@ upsert descriptor #105
   +    authorization:
   +      userName: root
   +    jobId: "1"
-  +    nameMapping: {}
+  +    nameMapping:
+  +      id: 105
+  +      name: f
   +    revertible: true
   +  dependedOnBy:
   +  - constraintIds:
@@ -262,7 +264,9 @@ upsert descriptor #105
   -    authorization:
   -      userName: root
   -    jobId: "1"
-  -    nameMapping: {}
+  -    nameMapping:
+  -      id: 105
+  -      name: f
   -    revertible: true
      dependedOnBy:
      - constraintIds:

pkg/sql/schemachanger/testdata/end_to_end/alter_table_add_check_with_seq_and_udt/alter_table_add_check_with_seq_and_udt.explain

@@ -10,7 +10,7 @@ Schema change plan for ALTER TABLE ‹defaultdb›.‹public›.‹t› ADD CHEC
  ├── StatementPhase
  │    └── Stage 1 of 1 in StatementPhase
  │         ├── 2 elements transitioning toward PUBLIC
- │         │    ├── ABSENT → WRITE_ONLY CheckConstraint:{DescID: 107 (t), ReferencedTypeIDs: [105 (typ), 106 (#106)], IndexID: 0, ConstraintID: 2 (check_i_j+), ReferencedSequenceIDs: [104 (s)]}
+ │         │    ├── ABSENT → WRITE_ONLY CheckConstraint:{DescID: 107 (t), ReferencedTypeIDs: [105 (typ), 106 (_typ)], IndexID: 0, ConstraintID: 2 (check_i_j+), ReferencedSequenceIDs: [104 (s)]}
  │         │    └── ABSENT → PUBLIC     ConstraintWithoutIndexName:{DescID: 107 (t), Name: "check_i_j", ConstraintID: 2 (check_i_j+)}
  │         └── 4 Mutation operations
  │              ├── AddCheckConstraint {"CheckExpr":"(i \u003e nextval(104...","ConstraintID":2,"TableID":107,"Validity":2}
@@ -20,13 +20,13 @@ Schema change plan for ALTER TABLE ‹defaultdb›.‹public›.‹t› ADD CHEC
  ├── PreCommitPhase
  │    ├── Stage 1 of 2 in PreCommitPhase
  │    │    ├── 2 elements transitioning toward PUBLIC
- │    │    │    ├── WRITE_ONLY → ABSENT CheckConstraint:{DescID: 107 (t), ReferencedTypeIDs: [105 (typ), 106 (#106)], IndexID: 0, ConstraintID: 2 (check_i_j+), ReferencedSequenceIDs: [104 (s)]}
+ │    │    │    ├── WRITE_ONLY → ABSENT CheckConstraint:{DescID: 107 (t), ReferencedTypeIDs: [105 (typ), 106 (_typ)], IndexID: 0, ConstraintID: 2 (check_i_j+), ReferencedSequenceIDs: [104 (s)]}
  │    │    │    └── PUBLIC     → ABSENT ConstraintWithoutIndexName:{DescID: 107 (t), Name: "check_i_j", ConstraintID: 2 (check_i_j+)}
  │    │    └── 1 Mutation operation
  │    │         └── UndoAllInTxnImmediateMutationOpSideEffects
  │    └── Stage 2 of 2 in PreCommitPhase
  │         ├── 2 elements transitioning toward PUBLIC
- │         │    ├── ABSENT → WRITE_ONLY CheckConstraint:{DescID: 107 (t), ReferencedTypeIDs: [105 (typ), 106 (#106)], IndexID: 0, ConstraintID: 2 (check_i_j+), ReferencedSequenceIDs: [104 (s)]}
+ │         │    ├── ABSENT → WRITE_ONLY CheckConstraint:{DescID: 107 (t), ReferencedTypeIDs: [105 (typ), 106 (_typ)], IndexID: 0, ConstraintID: 2 (check_i_j+), ReferencedSequenceIDs: [104 (s)]}
  │         │    └── ABSENT → PUBLIC     ConstraintWithoutIndexName:{DescID: 107 (t), Name: "check_i_j", ConstraintID: 2 (check_i_j+)}
  │         └── 9 Mutation operations
  │              ├── AddCheckConstraint {"CheckExpr":"(i \u003e nextval(104...","ConstraintID":2,"TableID":107,"Validity":2}
@@ -41,12 +41,12 @@ Schema change plan for ALTER TABLE ‹defaultdb›.‹public›.‹t› ADD CHEC
  └── PostCommitPhase
       ├── Stage 1 of 2 in PostCommitPhase
       │    ├── 1 element transitioning toward PUBLIC
-      │    │    └── WRITE_ONLY → VALIDATED CheckConstraint:{DescID: 107 (t), ReferencedTypeIDs: [105 (typ), 106 (#106)], IndexID: 0, ConstraintID: 2 (check_i_j+), ReferencedSequenceIDs: [104 (s)]}
+      │    │    └── WRITE_ONLY → VALIDATED CheckConstraint:{DescID: 107 (t), ReferencedTypeIDs: [105 (typ), 106 (_typ)], IndexID: 0, ConstraintID: 2 (check_i_j+), ReferencedSequenceIDs: [104 (s)]}
       │    └── 1 Validation operation
       │         └── ValidateConstraint {"ConstraintID":2,"TableID":107}
       └── Stage 2 of 2 in PostCommitPhase
            ├── 1 element transitioning toward PUBLIC
-           │    └── VALIDATED → PUBLIC CheckConstraint:{DescID: 107 (t), ReferencedTypeIDs: [105 (typ), 106 (#106)], IndexID: 0, ConstraintID: 2 (check_i_j+), ReferencedSequenceIDs: [104 (s)]}
+           │    └── VALIDATED → PUBLIC CheckConstraint:{DescID: 107 (t), ReferencedTypeIDs: [105 (typ), 106 (_typ)], IndexID: 0, ConstraintID: 2 (check_i_j+), ReferencedSequenceIDs: [104 (s)]}
            └── 6 Mutation operations
                 ├── MakeValidatedCheckConstraintPublic {"ConstraintID":2,"TableID":107}
                 ├── RemoveJobStateFromDescriptor {"DescriptorID":104}

pkg/sql/schemachanger/testdata/end_to_end/alter_table_add_check_with_seq_and_udt/alter_table_add_check_with_seq_and_udt.side_effects

@@ -155,7 +155,9 @@ upsert descriptor #106
   +    authorization:
   +      userName: root
   +    jobId: "1"
-  +    nameMapping: {}
+  +    nameMapping:
+  +      id: 106
+  +      name: _typ
   +    revertible: true
      id: 106
      kind: ALIAS
@@ -301,7 +303,9 @@ upsert descriptor #106
   -    authorization:
   -      userName: root
   -    jobId: "1"
-  -    nameMapping: {}
+  -    nameMapping:
+  -      id: 106
+  -      name: _typ
   -    revertible: true
      id: 106
      kind: ALIAS

pkg/sql/schemachanger/testdata/end_to_end/alter_table_add_check_with_seq_and_udt/alter_table_add_check_with_seq_and_udt__rollback_1_of_2.explain

@@ -11,7 +11,7 @@ Schema change plan for rolling back ALTER TABLE ‹defaultdb›.public.‹t› A
  └── PostCommitNonRevertiblePhase
       └── Stage 1 of 1 in PostCommitNonRevertiblePhase
            ├── 2 elements transitioning toward ABSENT
-           │    ├── WRITE_ONLY → ABSENT CheckConstraint:{DescID: 107 (t), ReferencedTypeIDs: [105 (typ), 106 (#106)], IndexID: 0, ConstraintID: 2 (check_i_j-), ReferencedSequenceIDs: [104 (s)]}
+           │    ├── WRITE_ONLY → ABSENT CheckConstraint:{DescID: 107 (t), ReferencedTypeIDs: [105 (typ), 106 (_typ)], IndexID: 0, ConstraintID: 2 (check_i_j-), ReferencedSequenceIDs: [104 (s)]}
            │    └── PUBLIC     → ABSENT ConstraintWithoutIndexName:{DescID: 107 (t), Name: "check_i_j", ConstraintID: 2 (check_i_j-)}
            └── 9 Mutation operations
                 ├── SetConstraintName {"ConstraintID":2,"Name":"crdb_internal_co...","TableID":107}

pkg/sql/schemachanger/testdata/end_to_end/alter_table_add_check_with_seq_and_udt/alter_table_add_check_with_seq_and_udt__rollback_2_of_2.explain

@@ -11,7 +11,7 @@ Schema change plan for rolling back ALTER TABLE ‹defaultdb›.public.‹t› A
  └── PostCommitNonRevertiblePhase
       └── Stage 1 of 1 in PostCommitNonRevertiblePhase
            ├── 2 elements transitioning toward ABSENT
-           │    ├── WRITE_ONLY → ABSENT CheckConstraint:{DescID: 107 (t), ReferencedTypeIDs: [105 (typ), 106 (#106)], IndexID: 0, ConstraintID: 2 (check_i_j-), ReferencedSequenceIDs: [104 (s)]}
+           │    ├── WRITE_ONLY → ABSENT CheckConstraint:{DescID: 107 (t), ReferencedTypeIDs: [105 (typ), 106 (_typ)], IndexID: 0, ConstraintID: 2 (check_i_j-), ReferencedSequenceIDs: [104 (s)]}
            │    └── PUBLIC     → ABSENT ConstraintWithoutIndexName:{DescID: 107 (t), Name: "check_i_j", ConstraintID: 2 (check_i_j-)}
            └── 9 Mutation operations
                 ├── SetConstraintName {"ConstraintID":2,"Name":"crdb_internal_co...","TableID":107}

pkg/sql/schemachanger/testdata/end_to_end/create_index/create_index.side_effects

@@ -135,7 +135,9 @@ upsert descriptor #105
   +    authorization:
   +      userName: root
   +    jobId: "1"
-  +    nameMapping: {}
+  +    nameMapping:
+  +      id: 105
+  +      name: _e
   +    revertible: true
      id: 105
      kind: ALIAS
@@ -420,7 +422,9 @@ upsert descriptor #105
   -    authorization:
   -      userName: root
   -    jobId: "1"
-  -    nameMapping: {}
+  -    nameMapping:
+  -      id: 105
+  -      name: _e
   -    revertible: true
      id: 105
      kind: ALIAS