From 9783626689f627cf4af395e28082b741b1999b76 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 --- .../schemachanger/scbuild/builder_state.go | 24 ++++++++++++------- .../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 +++++-- 7 files changed, 41 insertions(+), 21 deletions(-) diff --git a/pkg/sql/schemachanger/scbuild/builder_state.go b/pkg/sql/schemachanger/scbuild/builder_state.go index bb5707fb0ff8..7a9ff884838e 100644 --- a/pkg/sql/schemachanger/scbuild/builder_state.go +++ b/pkg/sql/schemachanger/scbuild/builder_state.go @@ -68,12 +68,25 @@ func (b *builderState) Ensure(e scpb.Element, target scpb.TargetStatus, meta scp default: panic(errors.AssertionFailedf("unsupported target %s", target.Status())) } + checkForConcurrentSchemaChanges := func() { + // 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)) + } + }) + } + 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. + checkForConcurrentSchemaChanges() b.addNewElementState(elementState{ element: e, initial: scpb.Status_ABSENT, @@ -89,14 +102,9 @@ 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)) - } - }) + // Check that there are no concurrent schema changes on the descriptors + // referenced by this element. + checkForConcurrentSchemaChanges() // 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. 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