Skip to content

Commit

Permalink
schemachanger: handle DROP following CREATE in-txn
Browse files Browse the repository at this point in the history 
This commit fixes a few bugs which prevented the following kind of
schema change from performing correctly:

    BEGIN;
    CREATE SCHEMA sc;
    DROP SCHEMA sc;

Informs cockroachdb#104123.

Release note: None
  • Loading branch information
Marius Posta committed Jun 8, 2023
1 parent 1824811 commit 073ca1f
Show file tree
Hide file tree
Showing 20 changed files with 343 additions and 60 deletions.
7 changes: 3 additions & 4 deletions pkg/sql/schemachanger/scbuild/builder_state.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,15 +85,14 @@ func (b *builderState) Ensure(e scpb.Element, target scpb.TargetStatus, meta scp
}
// Henceforth all possibilities lead to the target and metadata being
// overwritten. See below for explanations as to why this is legal.
oldTarget := dst.target
dst.target = target
dst.metadata = meta
oldTarget, oldStatementID := dst.target, dst.metadata.StatementID
dst.target, dst.metadata = target, meta
if dst.metadata.Size() == 0 {
// The element has never had a target set before.
// We can freely overwrite it.
return
}
if dst.metadata.StatementID == meta.StatementID {
if oldStatementID == meta.StatementID {
// The element has had a target set before, but it was in the same build.
// We can freely overwrite it or unset it.
if target.Status() == dst.initial {
Expand Down
63 changes: 60 additions & 3 deletions pkg/sql/schemachanger/scplan/internal/scgraph/graph.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,16 +134,73 @@ func New(cs scpb.CurrentState) (*Graph, error) {
g.depEdges = makeDepEdges(func(n *screl.Node) targetIdx {
return g.targetIdxMap[n.Target]
})
for i, status := range cs.Initial {
for i := range cs.Targets {
t := &cs.Targets[i]
var src scpb.Status
{
// Determine the status of the source node of the op-edge path
// which leads to the present target node.
if initial, current := cs.Initial[i], cs.Current[i]; initial == current {
// This is the straightforward case, which applies in all phases except
// the statement phase.
src = current
} else {
// Here we are in the statement phase and the plan include a pre-commit
// phase in which the element is transitioned from its current status
// back to its initial status and then onward to the target status.
// Those 3 corresponding nodes therefore need to be present in the
// graph. The source node therefore needs to be either the current or
// the initial status, depending on which is furthest from the target.
//
// Typically, that would be the initial status, however there are
// legitimate cases where the initial status is also the target status
// and in that case we need to pick the current status instead. This
// happens in explicit transactions where a DDL statement effectively
// undoes an earlier DDL statement in that transaction:
//
// BEGIN;
// CREATE SCHEMA sc;
// DROP SCHEMA sc;
//
// In this example when building the plan while executing the last
// statement, the Schema element will have:
// - an ABSENT target status, because we want to get rid of the
// newly-added schema,
// - an ABSENT initial status, because the schema didn't exist prior
// to this transaction,
// - a DESCRIPTOR_ADDING current status, because the previous
// statement has already executed its statement phase operations,
// in order to make the side-effects of the schema creation
// visible.
//
// The initial status has the convenient property in that it's either
// PUBLIC or ABSENT, because we don't allow concurrent schema changes.
// For this reason, we set the source node status based on whether
// the target is a no-op or not.
if initial == scpb.AsTargetStatus(t.TargetStatus).InitialStatus() {
// In this case, either the initial status is PUBLIC and the target
// is ABSENT, or the initial status is ABSENT and the target is
// PUBLIC or TRANSIENT_ABSENT. This is the straightforward sub-case
// where the current status is somewhere in between the initial
// and target statuses on the op-edge path.
src = initial
} else {
// This is the less straightforward sub-case where the target is a
// no-op with respect to the initial status. However, since we're in
// the statement phase, we still need to transition the element away
// from its current status.
src = current
}
}
}
if existing, ok := g.targetIdxMap[t]; ok {
return nil, errors.Errorf("invalid initial state contains duplicate target: %v and %v", *t, cs.Targets[existing])
}
idx := len(g.targets)
g.targetIdxMap[t] = targetIdx(idx)
g.targets = append(g.targets, t)
n := &screl.Node{Target: t, CurrentStatus: status}
g.targetNodes = append(g.targetNodes, map[scpb.Status]*screl.Node{status: n})
n := &screl.Node{Target: t, CurrentStatus: src}
g.targetNodes = append(g.targetNodes, map[scpb.Status]*screl.Node{src: n})
if err := g.entities.Insert(n); err != nil {
return nil, err
}
Expand Down
12 changes: 8 additions & 4 deletions pkg/sql/schemachanger/testdata/end_to_end/drop_multiple_columns_separate_statements
View file
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ write *eventpb.AlterTable to event log:
tag: ALTER TABLE
user: root
tableName: defaultdb.public.t
## StatementPhase stage 1 of 1 with 2 MutationType ops
## StatementPhase stage 1 of 1 with 3 MutationType ops
upsert descriptor #104
...
oid: 20
Expand Down Expand Up @@ -292,10 +292,14 @@ upsert descriptor #104
keySuffixColumnIds:
- 1
...
- 3
storeColumnNames:
partitioning: {}
sharded: {}
- storeColumnIds:
- - 3
- storeColumnNames:
- - k
+ - crdb_internal_column_3_name_placeholder
+ storeColumnIds: []
+ storeColumnNames: []
unique: true
version: 4
...
Expand Down
8 changes: 6 additions & 2 deletions .../schemachanger/testdata/explain/drop_multiple_columns_separate_statements.rollback_1_of_7
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,14 @@ EXPLAIN (ddl) rollback at post-commit stage 1 of 7;
Schema change plan for rolling back ALTER TABLE ‹defaultdb›.public.‹t› DROP COLUMN ‹k› CASCADE; following ALTER TABLE ‹defaultdb›.public.‹t› DROP COLUMN ‹j› CASCADE;
└── PostCommitNonRevertiblePhase
└── Stage 1 of 1 in PostCommitNonRevertiblePhase
├── 7 elements transitioning toward PUBLIC
├── 9 elements transitioning toward PUBLIC
│ ├── WRITE_ONLY → PUBLIC Column:{DescID: 104 (t), ColumnID: 3 (k+)}
│ ├── ABSENT → PUBLIC ColumnName:{DescID: 104 (t), Name: "k", ColumnID: 3 (k+)}
│ ├── WRITE_ONLY → PUBLIC Column:{DescID: 104 (t), ColumnID: 2 (j+)}
│ ├── WRITE_ONLY → PUBLIC Column:{DescID: 104 (t), ColumnID: 4 (crdb_internal_idx_expr+)}
│ ├── VALIDATED → PUBLIC SecondaryIndex:{DescID: 104 (t), IndexID: 2 (t_expr_k_idx+)}
│ ├── ABSENT → PUBLIC IndexColumn:{DescID: 104 (t), ColumnID: 3 (k+), IndexID: 3 (t_pkey-)}
│ ├── ABSENT → PUBLIC IndexColumn:{DescID: 104 (t), ColumnID: 3 (k+), IndexID: 4 (crdb_internal_index_4_name_placeholder)}
│ ├── ABSENT → PUBLIC ColumnName:{DescID: 104 (t), Name: "j", ColumnID: 2 (j+)}
│ └── ABSENT → PUBLIC ColumnName:{DescID: 104 (t), Name: "crdb_internal_idx_expr", ColumnID: 4 (crdb_internal_idx_expr+)}
├── 5 elements transitioning toward ABSENT
Expand All @@ -23,11 +25,13 @@ Schema change plan for rolling back ALTER TABLE ‹defaultdb›.public.‹t› D
│ ├── PUBLIC → ABSENT IndexColumn:{DescID: 104 (t), ColumnID: 1 (i), IndexID: 4 (crdb_internal_index_4_name_placeholder)}
│ ├── BACKFILL_ONLY → ABSENT PrimaryIndex:{DescID: 104 (t), IndexID: 3 (t_pkey-), ConstraintID: 2, TemporaryIndexID: 4 (crdb_internal_index_4_name_placeholder), SourceIndexID: 1 (t_pkey+)}
│ └── DELETE_ONLY → ABSENT TemporaryIndex:{DescID: 104 (t), IndexID: 4 (crdb_internal_index_4_name_placeholder), ConstraintID: 3, SourceIndexID: 1 (t_pkey+)}
└── 18 Mutation operations
└── 20 Mutation operations
├── SetColumnName {"ColumnID":3,"Name":"k","TableID":104}
├── MakeValidatedSecondaryIndexPublic {"IndexID":2,"TableID":104}
├── RefreshStats {"TableID":104}
├── AddColumnToIndex {"ColumnID":3,"IndexID":3,"Kind":2,"TableID":104}
├── RemoveColumnFromIndex {"ColumnID":1,"IndexID":4,"TableID":104}
├── AddColumnToIndex {"ColumnID":3,"IndexID":4,"Kind":2,"TableID":104}
├── SetColumnName {"ColumnID":2,"Name":"j","TableID":104}
├── SetColumnName {"ColumnID":4,"Name":"crdb_internal_id...","TableID":104}
├── MakeIndexAbsent {"IndexID":4,"TableID":104}
Expand Down
8 changes: 6 additions & 2 deletions .../schemachanger/testdata/explain/drop_multiple_columns_separate_statements.rollback_2_of_7
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,23 +9,27 @@ EXPLAIN (ddl) rollback at post-commit stage 2 of 7;
Schema change plan for rolling back ALTER TABLE ‹defaultdb›.public.‹t› DROP COLUMN ‹k› CASCADE; following ALTER TABLE ‹defaultdb›.public.‹t› DROP COLUMN ‹j› CASCADE;
└── PostCommitNonRevertiblePhase
├── Stage 1 of 2 in PostCommitNonRevertiblePhase
│ ├── 7 elements transitioning toward PUBLIC
│ ├── 9 elements transitioning toward PUBLIC
│ │ ├── WRITE_ONLY → PUBLIC Column:{DescID: 104 (t), ColumnID: 3 (k+)}
│ │ ├── ABSENT → PUBLIC ColumnName:{DescID: 104 (t), Name: "k", ColumnID: 3 (k+)}
│ │ ├── WRITE_ONLY → PUBLIC Column:{DescID: 104 (t), ColumnID: 2 (j+)}
│ │ ├── WRITE_ONLY → PUBLIC Column:{DescID: 104 (t), ColumnID: 4 (crdb_internal_idx_expr+)}
│ │ ├── VALIDATED → PUBLIC SecondaryIndex:{DescID: 104 (t), IndexID: 2 (t_expr_k_idx+)}
│ │ ├── ABSENT → PUBLIC IndexColumn:{DescID: 104 (t), ColumnID: 3 (k+), IndexID: 3 (t_pkey-)}
│ │ ├── ABSENT → PUBLIC IndexColumn:{DescID: 104 (t), ColumnID: 3 (k+), IndexID: 4 (crdb_internal_index_4_name_placeholder)}
│ │ ├── ABSENT → PUBLIC ColumnName:{DescID: 104 (t), Name: "j", ColumnID: 2 (j+)}
│ │ └── ABSENT → PUBLIC ColumnName:{DescID: 104 (t), Name: "crdb_internal_idx_expr", ColumnID: 4 (crdb_internal_idx_expr+)}
│ ├── 4 elements transitioning toward ABSENT
│ │ ├── PUBLIC → ABSENT IndexColumn:{DescID: 104 (t), ColumnID: 1 (i), IndexID: 3 (t_pkey-)}
│ │ ├── PUBLIC → ABSENT IndexColumn:{DescID: 104 (t), ColumnID: 1 (i), IndexID: 4 (crdb_internal_index_4_name_placeholder)}
│ │ ├── BACKFILL_ONLY → ABSENT PrimaryIndex:{DescID: 104 (t), IndexID: 3 (t_pkey-), ConstraintID: 2, TemporaryIndexID: 4 (crdb_internal_index_4_name_placeholder), SourceIndexID: 1 (t_pkey+)}
│ │ └── WRITE_ONLY → DELETE_ONLY TemporaryIndex:{DescID: 104 (t), IndexID: 4 (crdb_internal_index_4_name_placeholder), ConstraintID: 3, SourceIndexID: 1 (t_pkey+)}
│ └── 17 Mutation operations
│ └── 19 Mutation operations
│ ├── SetColumnName {"ColumnID":3,"Name":"k","TableID":104}
│ ├── MakeValidatedSecondaryIndexPublic {"IndexID":2,"TableID":104}
│ ├── RefreshStats {"TableID":104}
│ ├── AddColumnToIndex {"ColumnID":3,"IndexID":3,"Kind":2,"TableID":104}
│ ├── AddColumnToIndex {"ColumnID":3,"IndexID":4,"Kind":2,"TableID":104}
│ ├── SetColumnName {"ColumnID":2,"Name":"j","TableID":104}
│ ├── SetColumnName {"ColumnID":4,"Name":"crdb_internal_id...","TableID":104}
│ ├── MakeWriteOnlyIndexDeleteOnly {"IndexID":4,"TableID":104}
Expand Down
8 changes: 6 additions & 2 deletions .../schemachanger/testdata/explain/drop_multiple_columns_separate_statements.rollback_3_of_7
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,23 +9,27 @@ EXPLAIN (ddl) rollback at post-commit stage 3 of 7;
Schema change plan for rolling back ALTER TABLE ‹defaultdb›.public.‹t› DROP COLUMN ‹k› CASCADE; following ALTER TABLE ‹defaultdb›.public.‹t› DROP COLUMN ‹j› CASCADE;
└── PostCommitNonRevertiblePhase
├── Stage 1 of 2 in PostCommitNonRevertiblePhase
│ ├── 7 elements transitioning toward PUBLIC
│ ├── 9 elements transitioning toward PUBLIC
│ │ ├── WRITE_ONLY → PUBLIC Column:{DescID: 104 (t), ColumnID: 3 (k+)}
│ │ ├── ABSENT → PUBLIC ColumnName:{DescID: 104 (t), Name: "k", ColumnID: 3 (k+)}
│ │ ├── WRITE_ONLY → PUBLIC Column:{DescID: 104 (t), ColumnID: 2 (j+)}
│ │ ├── WRITE_ONLY → PUBLIC Column:{DescID: 104 (t), ColumnID: 4 (crdb_internal_idx_expr+)}
│ │ ├── VALIDATED → PUBLIC SecondaryIndex:{DescID: 104 (t), IndexID: 2 (t_expr_k_idx+)}
│ │ ├── ABSENT → PUBLIC IndexColumn:{DescID: 104 (t), ColumnID: 3 (k+), IndexID: 3 (t_pkey-)}
│ │ ├── ABSENT → PUBLIC IndexColumn:{DescID: 104 (t), ColumnID: 3 (k+), IndexID: 4 (crdb_internal_index_4_name_placeholder)}
│ │ ├── ABSENT → PUBLIC ColumnName:{DescID: 104 (t), Name: "j", ColumnID: 2 (j+)}
│ │ └── ABSENT → PUBLIC ColumnName:{DescID: 104 (t), Name: "crdb_internal_idx_expr", ColumnID: 4 (crdb_internal_idx_expr+)}
│ ├── 4 elements transitioning toward ABSENT
│ │ ├── PUBLIC → ABSENT IndexColumn:{DescID: 104 (t), ColumnID: 1 (i), IndexID: 3 (t_pkey-)}
│ │ ├── PUBLIC → ABSENT IndexColumn:{DescID: 104 (t), ColumnID: 1 (i), IndexID: 4 (crdb_internal_index_4_name_placeholder)}
│ │ ├── BACKFILL_ONLY → ABSENT PrimaryIndex:{DescID: 104 (t), IndexID: 3 (t_pkey-), ConstraintID: 2, TemporaryIndexID: 4 (crdb_internal_index_4_name_placeholder), SourceIndexID: 1 (t_pkey+)}
│ │ └── WRITE_ONLY → DELETE_ONLY TemporaryIndex:{DescID: 104 (t), IndexID: 4 (crdb_internal_index_4_name_placeholder), ConstraintID: 3, SourceIndexID: 1 (t_pkey+)}
│ └── 17 Mutation operations
│ └── 19 Mutation operations
│ ├── SetColumnName {"ColumnID":3,"Name":"k","TableID":104}
│ ├── MakeValidatedSecondaryIndexPublic {"IndexID":2,"TableID":104}
│ ├── RefreshStats {"TableID":104}
│ ├── AddColumnToIndex {"ColumnID":3,"IndexID":3,"Kind":2,"TableID":104}
│ ├── AddColumnToIndex {"ColumnID":3,"IndexID":4,"Kind":2,"TableID":104}
│ ├── SetColumnName {"ColumnID":2,"Name":"j","TableID":104}
│ ├── SetColumnName {"ColumnID":4,"Name":"crdb_internal_id...","TableID":104}
│ ├── MakeWriteOnlyIndexDeleteOnly {"IndexID":4,"TableID":104}
Expand Down
8 changes: 6 additions & 2 deletions .../schemachanger/testdata/explain/drop_multiple_columns_separate_statements.rollback_4_of_7
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,24 +9,28 @@ EXPLAIN (ddl) rollback at post-commit stage 4 of 7;
Schema change plan for rolling back ALTER TABLE ‹defaultdb›.public.‹t› DROP COLUMN ‹k› CASCADE; following ALTER TABLE ‹defaultdb›.public.‹t› DROP COLUMN ‹j› CASCADE;
└── PostCommitNonRevertiblePhase
├── Stage 1 of 2 in PostCommitNonRevertiblePhase
│ ├── 7 elements transitioning toward PUBLIC
│ ├── 9 elements transitioning toward PUBLIC
│ │ ├── WRITE_ONLY → PUBLIC Column:{DescID: 104 (t), ColumnID: 3 (k+)}
│ │ ├── ABSENT → PUBLIC ColumnName:{DescID: 104 (t), Name: "k", ColumnID: 3 (k+)}
│ │ ├── WRITE_ONLY → PUBLIC Column:{DescID: 104 (t), ColumnID: 2 (j+)}
│ │ ├── WRITE_ONLY → PUBLIC Column:{DescID: 104 (t), ColumnID: 4 (crdb_internal_idx_expr+)}
│ │ ├── VALIDATED → PUBLIC SecondaryIndex:{DescID: 104 (t), IndexID: 2 (t_expr_k_idx+)}
│ │ ├── ABSENT → PUBLIC IndexColumn:{DescID: 104 (t), ColumnID: 3 (k+), IndexID: 3 (t_pkey-)}
│ │ ├── ABSENT → PUBLIC IndexColumn:{DescID: 104 (t), ColumnID: 3 (k+), IndexID: 4 (crdb_internal_index_4_name_placeholder)}
│ │ ├── ABSENT → PUBLIC ColumnName:{DescID: 104 (t), Name: "j", ColumnID: 2 (j+)}
│ │ └── ABSENT → PUBLIC ColumnName:{DescID: 104 (t), Name: "crdb_internal_idx_expr", ColumnID: 4 (crdb_internal_idx_expr+)}
│ ├── 4 elements transitioning toward ABSENT
│ │ ├── PUBLIC → ABSENT IndexColumn:{DescID: 104 (t), ColumnID: 1 (i), IndexID: 3 (t_pkey-)}
│ │ ├── PUBLIC → ABSENT IndexColumn:{DescID: 104 (t), ColumnID: 1 (i), IndexID: 4 (crdb_internal_index_4_name_placeholder)}
│ │ ├── DELETE_ONLY → ABSENT PrimaryIndex:{DescID: 104 (t), IndexID: 3 (t_pkey-), ConstraintID: 2, TemporaryIndexID: 4 (crdb_internal_index_4_name_placeholder), SourceIndexID: 1 (t_pkey+)}
│ │ └── WRITE_ONLY → DELETE_ONLY TemporaryIndex:{DescID: 104 (t), IndexID: 4 (crdb_internal_index_4_name_placeholder), ConstraintID: 3, SourceIndexID: 1 (t_pkey+)}
│ └── 17 Mutation operations
│ └── 19 Mutation operations
│ ├── SetColumnName {"ColumnID":3,"Name":"k","TableID":104}
│ ├── MakeValidatedSecondaryIndexPublic {"IndexID":2,"TableID":104}
│ ├── RefreshStats {"TableID":104}
│ ├── RemoveColumnFromIndex {"ColumnID":1,"IndexID":3,"TableID":104}
│ ├── AddColumnToIndex {"ColumnID":3,"IndexID":3,"Kind":2,"TableID":104}
│ ├── AddColumnToIndex {"ColumnID":3,"IndexID":4,"Kind":2,"TableID":104}
│ ├── SetColumnName {"ColumnID":2,"Name":"j","TableID":104}
│ ├── SetColumnName {"ColumnID":4,"Name":"crdb_internal_id...","TableID":104}
│ ├── MakeIndexAbsent {"IndexID":3,"TableID":104}
Expand Down
Loading

0 comments on commit 073ca1f

Please sign in to comment.