From b9d73b6edd525352bbd4e26c077e1f9deb7ae7e4 Mon Sep 17 00:00:00 2001 From: Marius Posta Date: Mon, 17 Jul 2023 14:37:32 -0400 Subject: [PATCH] scbuild: fix concurrent schema change verification bug. Previously, we didn't check for concurrent schema changes on targets set on elements that the builder state didn't know about yet. This patch fixes this oversight. This regression was recently introduced by #106175. Fixes: #106487 Release note: None --- pkg/sql/schemachanger/scbuild/build.go | 2 +- .../schemachanger/scbuild/builder_state.go | 45 ++++++++++++++----- .../alter_table_add_check_udf.side_effects | 8 +++- ...r_table_add_check_with_seq_and_udt.explain | 10 ++--- ...le_add_check_with_seq_and_udt.side_effects | 8 +++- ..._with_seq_and_udt__rollback_1_of_2.explain | 2 +- ..._with_seq_and_udt__rollback_2_of_2.explain | 2 +- .../create_index/create_index.side_effects | 8 +++- 8 files changed, 59 insertions(+), 26 deletions(-) diff --git a/pkg/sql/schemachanger/scbuild/build.go b/pkg/sql/schemachanger/scbuild/build.go index 66a939bc78af..41476f2d364d 100644 --- a/pkg/sql/schemachanger/scbuild/build.go +++ b/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{ diff --git a/pkg/sql/schemachanger/scbuild/builder_state.go b/pkg/sql/schemachanger/scbuild/builder_state.go index bb5707fb0ff8..a5ea84d7d9d7 100644 --- a/pkg/sql/schemachanger/scbuild/builder_state.go +++ b/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 { diff --git a/pkg/sql/schemachanger/testdata/end_to_end/alter_table_add_check_udf/alter_table_add_check_udf.side_effects b/pkg/sql/schemachanger/testdata/end_to_end/alter_table_add_check_udf/alter_table_add_check_udf.side_effects index 33e4976a4070..76c944dff14a 100644 --- a/pkg/sql/schemachanger/testdata/end_to_end/alter_table_add_check_udf/alter_table_add_check_udf.side_effects +++ b/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: diff --git a/pkg/sql/schemachanger/testdata/end_to_end/alter_table_add_check_with_seq_and_udt/alter_table_add_check_with_seq_and_udt.explain b/pkg/sql/schemachanger/testdata/end_to_end/alter_table_add_check_with_seq_and_udt/alter_table_add_check_with_seq_and_udt.explain index 542e57b37abc..cab790d5109d 100644 --- a/pkg/sql/schemachanger/testdata/end_to_end/alter_table_add_check_with_seq_and_udt/alter_table_add_check_with_seq_and_udt.explain +++ b/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} diff --git a/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 b/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 index c79576466884..237be13e28da 100644 --- a/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 +++ b/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 diff --git a/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 b/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 index 484f28e95fb6..738ce111b0b8 100644 --- a/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 +++ b/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} diff --git a/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 b/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 index 77f62eb66577..c6c023e1d6ef 100644 --- a/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 +++ b/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} diff --git a/pkg/sql/schemachanger/testdata/end_to_end/create_index/create_index.side_effects b/pkg/sql/schemachanger/testdata/end_to_end/create_index/create_index.side_effects index 5d486d1d5823..f10d7f1134ac 100644 --- a/pkg/sql/schemachanger/testdata/end_to_end/create_index/create_index.side_effects +++ b/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