sql/schemachanger/scplan: ensure revertibility applies correctly

Without this patch, it would not apply at Statement or PreCommit Release note: None
cockroachdb · Jul 14, 2022 · 3c0c52b · 3c0c52b
1 parent ea759fe
commit 3c0c52b
Show file tree

Hide file tree

Showing 4 changed files with 53 additions and 38 deletions.
diff --git a/pkg/sql/schemachanger/scplan/internal/scstage/build.go b/pkg/sql/schemachanger/scplan/internal/scstage/build.go
@@ -283,7 +283,9 @@ func (sb stageBuilder) isOutgoingOpEdgeAllowed(e *scgraph.OpEdge) bool {
 	if !e.IsPhaseSatisfied(sb.bs.phase) {
 		return false
 	}
-	if !sb.bc.isRevertibilityIgnored && sb.bs.phase == scop.PostCommitPhase && !e.Revertible() {
+	if !sb.bc.isRevertibilityIgnored &&
+		sb.bs.phase < scop.PostCommitNonRevertiblePhase &&
+		!e.Revertible() {
 		return false
 	}
 	return true

diff --git a/pkg/sql/schemachanger/scplan/testdata/drop_index b/pkg/sql/schemachanger/scplan/testdata/drop_index
@@ -339,14 +339,12 @@ DROP INDEX idx2 CASCADE
 ops
 DROP INDEX idx3 CASCADE
 ----
-StatementPhase stage 1 of 1 with 7 MutationType ops
+StatementPhase stage 1 of 1 with 5 MutationType ops
   transitions:
     [[Column:{DescID: 104, ColumnID: 5}, ABSENT], PUBLIC] -> WRITE_ONLY
     [[ColumnName:{DescID: 104, Name: crdb_internal_i_shard_16, ColumnID: 5}, ABSENT], PUBLIC] -> ABSENT
     [[SecondaryIndex:{DescID: 104, IndexID: 6, ConstraintID: 0}, ABSENT], PUBLIC] -> VALIDATED
     [[IndexName:{DescID: 104, Name: idx3, IndexID: 6}, ABSENT], PUBLIC] -> ABSENT
-    [[CheckConstraint:{DescID: 104, ConstraintID: 4}, ABSENT], PUBLIC] -> ABSENT
-    [[ConstraintName:{DescID: 104, Name: check_crdb_internal_i_shard_16, ConstraintID: 4}, ABSENT], PUBLIC] -> ABSENT
   ops:
     *scop.MakeDroppedColumnDeleteAndWriteOnly
       ColumnID: 5
@@ -378,11 +376,6 @@ StatementPhase stage 1 of 1 with 7 MutationType ops
       IndexID: 6
       Name: crdb_internal_index_6_name_placeholder
       TableID: 104
-    *scop.RemoveCheckConstraint
-      ConstraintID: 4
-      TableID: 104
-    *scop.NotImplemented
-      ElementType: scpb.ConstraintName
 PreCommitPhase stage 1 of 1 with 2 MutationType ops
   transitions:
   ops:
@@ -396,22 +389,29 @@ PreCommitPhase stage 1 of 1 with 2 MutationType ops
       - 104
       JobID: 1
       NonCancelable: true
-      RunningStatus: PostCommitNonRevertiblePhase stage 1 of 2 with 5 MutationType ops pending
+      RunningStatus: PostCommitNonRevertiblePhase stage 1 of 2 with 7 MutationType ops pending
       Statements:
       - statement: DROP INDEX idx3 CASCADE
         redactedstatement: DROP INDEX ‹defaultdb›.public.‹t1›@‹idx3› CASCADE
         statementtag: DROP INDEX
-PostCommitNonRevertiblePhase stage 1 of 2 with 7 MutationType ops
+PostCommitNonRevertiblePhase stage 1 of 2 with 9 MutationType ops
   transitions:
     [[Column:{DescID: 104, ColumnID: 5}, ABSENT], WRITE_ONLY] -> DELETE_ONLY
     [[IndexColumn:{DescID: 104, ColumnID: 5, IndexID: 6}, ABSENT], PUBLIC] -> ABSENT
     [[IndexColumn:{DescID: 104, ColumnID: 1, IndexID: 6}, ABSENT], PUBLIC] -> ABSENT
     [[IndexColumn:{DescID: 104, ColumnID: 3, IndexID: 6}, ABSENT], PUBLIC] -> ABSENT
     [[SecondaryIndex:{DescID: 104, IndexID: 6, ConstraintID: 0}, ABSENT], VALIDATED] -> DELETE_ONLY
+    [[CheckConstraint:{DescID: 104, ConstraintID: 4}, ABSENT], PUBLIC] -> ABSENT
+    [[ConstraintName:{DescID: 104, Name: check_crdb_internal_i_shard_16, ConstraintID: 4}, ABSENT], PUBLIC] -> ABSENT
   ops:
     *scop.MakeDroppedColumnDeleteOnly
       ColumnID: 5
       TableID: 104
+    *scop.RemoveCheckConstraint
+      ConstraintID: 4
+      TableID: 104
+    *scop.NotImplemented
+      ElementType: scpb.ConstraintName
     *scop.MakeDroppedIndexDeleteOnly
       IndexID: 6
       TableID: 104

diff --git a/pkg/sql/schemachanger/scplan/testdata/drop_owned_by b/pkg/sql/schemachanger/scplan/testdata/drop_owned_by
@@ -17,11 +17,9 @@ CREATE VIEW s.v2 AS (SELECT 'a'::s.typ::string AS k, name FROM s.v1);
 ops
 DROP OWNED BY r
 ----
-StatementPhase stage 1 of 1 with 11 MutationType ops
+StatementPhase stage 1 of 1 with 9 MutationType ops
   transitions:
-    [[UserPrivileges:{DescID: 100, Name: r}, ABSENT], PUBLIC] -> ABSENT
     [[Schema:{DescID: 105}, ABSENT], PUBLIC] -> OFFLINE
-    [[UserPrivileges:{DescID: 104, Name: r}, ABSENT], PUBLIC] -> ABSENT
     [[Sequence:{DescID: 106}, ABSENT], PUBLIC] -> OFFLINE
     [[Table:{DescID: 109}, ABSENT], PUBLIC] -> OFFLINE
     [[Column:{DescID: 109, ColumnID: 1}, ABSENT], PUBLIC] -> WRITE_ONLY
@@ -50,15 +48,9 @@ StatementPhase stage 1 of 1 with 11 MutationType ops
     [[Column:{DescID: 113, ColumnID: 4294967295}, ABSENT], PUBLIC] -> WRITE_ONLY
     [[Column:{DescID: 113, ColumnID: 4294967294}, ABSENT], PUBLIC] -> WRITE_ONLY
   ops:
-    *scop.RemoveUserPrivileges
-      DescID: 100
-      User: r
     *scop.MarkDescriptorAsOffline
       DescID: 105
       Reason: DROP OWNED BY r
-    *scop.RemoveUserPrivileges
-      DescID: 104
-      User: r
     *scop.MarkDescriptorAsOffline
       DescID: 106
       Reason: DROP OWNED BY r
@@ -136,21 +128,23 @@ PreCommitPhase stage 1 of 1 with 12 MutationType ops
       - 113
       JobID: 1
       NonCancelable: true
-      RunningStatus: PostCommitNonRevertiblePhase stage 1 of 2 with 32 MutationType ops
+      RunningStatus: PostCommitNonRevertiblePhase stage 1 of 2 with 34 MutationType ops
         pending
       Statements:
       - statement: DROP OWNED BY r
         redactedstatement: DROP OWNED BY r
         statementtag: DROP OWNED BY
-PostCommitNonRevertiblePhase stage 1 of 2 with 44 MutationType ops
+PostCommitNonRevertiblePhase stage 1 of 2 with 46 MutationType ops
   transitions:
+    [[UserPrivileges:{DescID: 100, Name: r}, ABSENT], PUBLIC] -> ABSENT
     [[Namespace:{DescID: 105, Name: s, ReferencedDescID: 100}, ABSENT], PUBLIC] -> ABSENT
     [[Owner:{DescID: 105}, ABSENT], PUBLIC] -> ABSENT
     [[UserPrivileges:{DescID: 105, Name: admin}, ABSENT], PUBLIC] -> ABSENT
     [[UserPrivileges:{DescID: 105, Name: r}, ABSENT], PUBLIC] -> ABSENT
     [[UserPrivileges:{DescID: 105, Name: root}, ABSENT], PUBLIC] -> ABSENT
     [[Schema:{DescID: 105}, ABSENT], OFFLINE] -> DROPPED
     [[SchemaParent:{DescID: 105, ReferencedDescID: 100}, ABSENT], PUBLIC] -> ABSENT
+    [[UserPrivileges:{DescID: 104, Name: r}, ABSENT], PUBLIC] -> ABSENT
     [[Namespace:{DescID: 106, Name: sq, ReferencedDescID: 100}, ABSENT], PUBLIC] -> ABSENT
     [[Owner:{DescID: 106}, ABSENT], PUBLIC] -> ABSENT
     [[UserPrivileges:{DescID: 106, Name: admin}, ABSENT], PUBLIC] -> ABSENT
@@ -276,12 +270,18 @@ PostCommitNonRevertiblePhase stage 1 of 2 with 44 MutationType ops
     [[ColumnName:{DescID: 113, Name: tableoid, ColumnID: 4294967294}, ABSENT], PUBLIC] -> ABSENT
     [[ColumnType:{DescID: 113, ColumnFamilyID: 0, ColumnID: 4294967294}, ABSENT], PUBLIC] -> ABSENT
   ops:
+    *scop.RemoveUserPrivileges
+      DescID: 100
+      User: r
     *scop.MarkDescriptorAsDropped
       DescID: 105
     *scop.RemoveSchemaParent
       Parent:
         ParentDatabaseID: 100
         SchemaID: 105
+    *scop.RemoveUserPrivileges
+      DescID: 104
+      User: r
     *scop.MarkDescriptorAsDropped
       DescID: 106
     *scop.RemoveAllTableComments

diff --git a/pkg/sql/schemachanger/testdata/drop_index_hash_sharded_index b/pkg/sql/schemachanger/testdata/drop_index_hash_sharded_index
@@ -12,21 +12,19 @@ begin transaction #1
 # begin StatementPhase
 checking for feature: DROP INDEX
 increment telemetry for sql.schema.drop_index
-## StatementPhase stage 1 of 1 with 7 MutationType ops
+## StatementPhase stage 1 of 1 with 5 MutationType ops
 upsert descriptor #104
-   table:
-  -  checks:
-  -  - columnIds:
-  -    - 3
-  -    constraintId: 4
+  ...
+       - 3
+       constraintId: 4
   -    expr: crdb_internal_j_shard_16 IN (0:::INT8, 1:::INT8, 2:::INT8, 3:::INT8, 4:::INT8,
   -      5:::INT8, 6:::INT8, 7:::INT8, 8:::INT8, 9:::INT8, 10:::INT8, 11:::INT8, 12:::INT8,
   -      13:::INT8, 14:::INT8, 15:::INT8)
-  -    hidden: true
-  -    name: check_crdb_internal_j_shard_16
-  +  checks: []
-     columns:
-     - id: 1
+  +    expr: crdb_internal_column_3_name_placeholder IN (0:::INT8, 1:::INT8, 2:::INT8,
+  +      3:::INT8, 4:::INT8, 5:::INT8, 6:::INT8, 7:::INT8, 8:::INT8, 9:::INT8, 10:::INT8,
+  +      11:::INT8, 12:::INT8, 13:::INT8, 14:::INT8, 15:::INT8)
+       hidden: true
+       name: check_crdb_internal_j_shard_16
   ...
          oid: 20
          width: 64
@@ -145,8 +143,8 @@ upsert descriptor #104
   +    - PUBLIC
   +    - VALIDATED
   +    - ABSENT
-  +    - ABSENT
-  +    - ABSENT
+  +    - PUBLIC
+  +    - PUBLIC
   +    jobId: "1"
   +    relevantStatements:
   +    - statement:
@@ -298,8 +296,21 @@ notified job registry to adopt jobs: [1]
 begin transaction #2
 commit transaction #2
 begin transaction #3
-## PostCommitNonRevertiblePhase stage 1 of 2 with 7 MutationType ops
+## PostCommitNonRevertiblePhase stage 1 of 2 with 9 MutationType ops
 upsert descriptor #104
+   table:
+  -  checks:
+  -  - columnIds:
+  -    - 3
+  -    constraintId: 4
+  -    expr: crdb_internal_column_3_name_placeholder IN (0:::INT8, 1:::INT8, 2:::INT8,
+  -      3:::INT8, 4:::INT8, 5:::INT8, 6:::INT8, 7:::INT8, 8:::INT8, 9:::INT8, 10:::INT8,
+  -      11:::INT8, 12:::INT8, 13:::INT8, 14:::INT8, 15:::INT8)
+  -    hidden: true
+  -    name: check_crdb_internal_j_shard_16
+  +  checks: []
+     columns:
+     - id: 1
   ...
          userName: root
        currentStatuses:
@@ -312,8 +323,10 @@ upsert descriptor #104
   -    - PUBLIC
   -    - VALIDATED
        - ABSENT
-       - ABSENT
-       - ABSENT
+  -    - PUBLIC
+  -    - PUBLIC
+  +    - ABSENT
+  +    - ABSENT
   +    - DELETE_ONLY
   +    - ABSENT
   +    - ABSENT