Merge cockroachdb#97748

97748: sql/schemachanger/scbuild: properly rewrite exprs in partial predicates r=ajwerner a=ajwerner Before this change we were throwing away some standard rewriting we want to do to table expressions for the case of partial index predicates. This resulted in badness when attempting to backfill the index. No release note because this feature is new in 23.1 Fixes: cockroachdb#97551 Release note: None Co-authored-by: ajwerner <[email protected]>
nicktrav · Feb 28, 2023 · cfe009c · cfe009c
2 parents 46b1afa + 7192416
commit cfe009c
Show file tree

Hide file tree

Showing 24 changed files with 500 additions and 71 deletions.
diff --git a/pkg/sql/logictest/testdata/logic_test/partial_index b/pkg/sql/logictest/testdata/logic_test/partial_index
@@ -2034,3 +2034,48 @@ DROP INDEX t96924_idx_1
 
 statement ok
 ALTER TABLE t96924 ALTER PRIMARY KEY USING COLUMNS (a);
+
+subtest enum_cast_references
+
+statement ok
+CREATE TYPE enum97551 AS ENUM ('a', 'b', 'c');
+CREATE TABLE t97551 (i INT PRIMARY KEY, j STRING);
+
+# We shouldn't be allowed to create this index because the cast from string
+# to an enum is not immutable. However, we can because of  #68672, so we
+# exercise that here. There's another set of statements below which can
+# continue to exist after that bug is fixed and also protect against the
+# underlying regression.
+statement ok
+CREATE INDEX idx97551 ON t97551 (j) WHERE (j::enum97551 = 'a'::enum97551);
+
+statement error pgcode 2BP01 cannot drop type "enum97551" because other objects \(\[test\.public\.t97551\]\) still depend on it
+DROP TYPE enum97551;
+
+statement ok
+DROP INDEX idx97551;
+
+statement ok
+DROP TYPE enum97551;
+
+# Do the same dance but with an expression that is actually immutable.
+
+statement ok
+CREATE TYPE enum97551 AS ENUM ('a', 'b', 'c');
+
+statement ok
+ALTER TABLE t97551 ADD COLUMN e enum97551;
+
+statement ok
+CREATE INDEX idx97551 ON t97551 (e) WHERE (e = 'a'::enum97551);
+
+statement error pgcode 2BP01 cannot drop type "enum97551" because other objects \(\[test\.public\.t97551\]\) still depend on it
+DROP TYPE enum97551;
+
+statement ok
+DROP INDEX idx97551;
+
+statement ok
+DROP TABLE t97551;
+DROP TYPE enum97551;
+
diff --git a/pkg/sql/schemachanger/scbuild/builder_state.go b/pkg/sql/schemachanger/scbuild/builder_state.go
@@ -633,7 +633,7 @@ func (b *builderState) ComputedColumnExpression(tbl *scpb.Table, d *tree.ColumnT
 // PartialIndexPredicateExpression implements the scbuildstmt.TableHelpers interface.
 func (b *builderState) PartialIndexPredicateExpression(
 	tableID catid.DescID, expr tree.Expr,
-) *scpb.Expression {
+) tree.Expr {
 	// Ensure that an namespace entry exists for the table.
 	_, _, ns := scpb.FindNamespace(b.QueryByID(tableID))
 	if ns == nil {
@@ -647,7 +647,7 @@ func (b *builderState) PartialIndexPredicateExpression(
 	// TODO(fqazi): this doesn't work when referencing newly added columns, this
 	// is not problematic today since declarative schema changer is only enabled
 	// for single statement transactions.
-	_, err := schemaexpr.ValidatePartialIndexPredicate(
+	validatedExpr, err := schemaexpr.ValidatePartialIndexPredicate(
 		b.ctx,
 		b.descCache[tableID].desc.(catalog.TableDescriptor),
 		expr,
@@ -657,7 +657,11 @@ func (b *builderState) PartialIndexPredicateExpression(
 	if err != nil {
 		panic(err)
 	}
-	return b.WrapExpression(tableID, expr)
+	parsedExpr, err := parser.ParseExpr(validatedExpr)
+	if err != nil {
+		panic(err)
+	}
+	return parsedExpr
 }
 
 var _ scbuildstmt.ElementReferences = (*builderState)(nil)

diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_index.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_index.go
@@ -930,7 +930,7 @@ func maybeAddIndexPredicate(b BuildCtx, n *tree.CreateIndex, idxSpec *indexSpec)
 		return
 	}
 	expr := b.PartialIndexPredicateExpression(idxSpec.secondary.TableID, n.Predicate)
-	idxSpec.secondary.EmbeddedExpr = expr
+	idxSpec.secondary.EmbeddedExpr = b.WrapExpression(idxSpec.secondary.TableID, expr)
 	b.IncrementSchemaChangeIndexCounter("partial")
 	if n.Inverted {
 		b.IncrementSchemaChangeIndexCounter("partial_inverted")

diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/dependencies.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/dependencies.go
@@ -254,7 +254,7 @@ type TableHelpers interface {
 	// wrapped expression
 	PartialIndexPredicateExpression(
 		tableID catid.DescID, expr tree.Expr,
-	) *scpb.Expression
+	) tree.Expr
 
 	// IsTableEmpty returns if the table is empty or not.
 	IsTableEmpty(tbl *scpb.Table) bool

diff --git a/pkg/sql/schemachanger/scbuild/testdata/drop_index b/pkg/sql/schemachanger/scbuild/testdata/drop_index
@@ -42,7 +42,7 @@ DROP INDEX idx2 CASCADE
 - [[IndexColumn:{DescID: 104, ColumnID: 3, IndexID: 4}, ABSENT], PUBLIC]
   {columnId: 3, indexId: 4, kind: KEY_SUFFIX, tableId: 104}
 - [[SecondaryIndex:{DescID: 104, IndexID: 4, ConstraintID: 0}, ABSENT], PUBLIC]
-  {expr: i > 0, indexId: 4, isCreatedExplicitly: true, referencedColumnIds: [1], tableId: 104}
+  {expr: 'i > 0:::INT8', indexId: 4, isCreatedExplicitly: true, referencedColumnIds: [1], tableId: 104}
 - [[IndexName:{DescID: 104, Name: idx2, IndexID: 4}, ABSENT], PUBLIC]
   {indexId: 4, name: idx2, tableId: 104}
 - [[IndexData:{DescID: 104, IndexID: 4}, ABSENT], PUBLIC]

diff --git a/pkg/sql/schemachanger/scplan/testdata/drop_table b/pkg/sql/schemachanger/scplan/testdata/drop_table
@@ -155,7 +155,7 @@ PreCommitPhase stage 1 of 2 with 1 MutationType op
   ops:
     *scop.UndoAllInTxnImmediateMutationOpSideEffects
       {}
-PreCommitPhase stage 2 of 2 with 119 MutationType ops
+PreCommitPhase stage 2 of 2 with 120 MutationType ops
   transitions:
     [[Namespace:{DescID: 109, Name: shipments, ReferencedDescID: 100}, ABSENT], PUBLIC] -> ABSENT
     [[Owner:{DescID: 109}, ABSENT], PUBLIC] -> ABSENT
@@ -542,6 +542,11 @@ PreCommitPhase stage 2 of 2 with 119 MutationType ops
     *scop.RemoveDroppedIndexPartialPredicate
       IndexID: 2
       TableID: 109
+    *scop.UpdateTableBackReferencesInTypes
+      BackReferencedTableID: 109
+      TypeIDs:
+      - 107
+      - 108
     *scop.SetIndexName
       IndexID: 2
       Name: crdb_internal_index_2_name_placeholder

diff --git a/pkg/sql/schemachanger/testdata/end_to_end/create_index b/pkg/sql/schemachanger/testdata/end_to_end/create_index
@@ -57,7 +57,19 @@ write *eventpb.CreateIndex to event log:
     tag: CREATE INDEX
     user: root
   tableName: defaultdb.public.t
-## StatementPhase stage 1 of 1 with 8 MutationType ops
+## StatementPhase stage 1 of 1 with 9 MutationType ops
+upsert descriptor #104
+  ...
+     referencingDescriptorIds:
+     - 106
+  -  version: "2"
+  +  version: "3"
+upsert descriptor #105
+  ...
+     referencingDescriptorIds:
+     - 106
+  -  version: "2"
+  +  version: "3"
 upsert descriptor #106
   ...
      id: 106
@@ -81,7 +93,7 @@ upsert descriptor #106
   +      - 1
   +      name: idx1
   +      partitioning: {}
-  +      predicate: (v = 'a')
+  +      predicate: v = b'@':::@100104
   +      sharded: {}
   +      storeColumnNames: []
   +      version: 4
@@ -129,7 +141,38 @@ upsert descriptor #106
 ## PreCommitPhase stage 1 of 2 with 1 MutationType op
 undo all catalog changes within txn #1
 persist all catalog changes to storage
-## PreCommitPhase stage 2 of 2 with 12 MutationType ops
+## PreCommitPhase stage 2 of 2 with 15 MutationType ops
+upsert descriptor #104
+   type:
+     arrayTypeId: 105
+  +  declarativeSchemaChangerState:
+  +    authorization:
+  +      userName: root
+  +    jobId: "1"
+  +    revertible: true
+     enumMembers:
+     - logicalRepresentation: a
+  ...
+     referencingDescriptorIds:
+     - 106
+  -  version: "2"
+  +  version: "3"
+upsert descriptor #105
+  ...
+       family: ArrayFamily
+       oid: 100105
+  +  declarativeSchemaChangerState:
+  +    authorization:
+  +      userName: root
+  +    jobId: "1"
+  +    revertible: true
+     id: 105
+     kind: ALIAS
+  ...
+     referencingDescriptorIds:
+     - 106
+  -  version: "2"
+  +  version: "3"
 upsert descriptor #106
   ...
      createAsOfTime:
@@ -172,7 +215,7 @@ upsert descriptor #106
   +      - 1
   +      name: idx1
   +      partitioning: {}
-  +      predicate: (v = 'a')
+  +      predicate: v = b'@':::@100104
   +      sharded: {}
   +      storeColumnNames: []
   +      version: 4
@@ -217,15 +260,27 @@ upsert descriptor #106
   +  version: "2"
 persist all catalog changes to storage
 create job #1 (non-cancelable: false): "CREATE INDEX idx1 ON defaultdb.public.t (v) WHERE (v = 'a')"
-  descriptor IDs: [106]
+  descriptor IDs: [104 105 106]
 # end PreCommitPhase
 commit transaction #1
 notified job registry to adopt jobs: [1]
 # begin PostCommitPhase
 begin transaction #2
 commit transaction #2
 begin transaction #3
-## PostCommitPhase stage 1 of 7 with 3 MutationType ops
+## PostCommitPhase stage 1 of 7 with 5 MutationType ops
+upsert descriptor #104
+  ...
+     referencingDescriptorIds:
+     - 106
+  -  version: "3"
+  +  version: "4"
+upsert descriptor #105
+  ...
+     referencingDescriptorIds:
+     - 106
+  -  version: "3"
+  +  version: "4"
 upsert descriptor #106
   ...
          version: 4
@@ -247,7 +302,19 @@ begin transaction #4
 backfill indexes [2] from index #1 in table #106
 commit transaction #4
 begin transaction #5
-## PostCommitPhase stage 3 of 7 with 3 MutationType ops
+## PostCommitPhase stage 3 of 7 with 5 MutationType ops
+upsert descriptor #104
+  ...
+     referencingDescriptorIds:
+     - 106
+  -  version: "4"
+  +  version: "5"
+upsert descriptor #105
+  ...
+     referencingDescriptorIds:
+     - 106
+  -  version: "4"
+  +  version: "5"
 upsert descriptor #106
   ...
          version: 4
@@ -265,7 +332,19 @@ persist all catalog changes to storage
 update progress of schema change job #1: "PostCommitPhase stage 4 of 7 with 1 MutationType op pending"
 commit transaction #5
 begin transaction #6
-## PostCommitPhase stage 4 of 7 with 3 MutationType ops
+## PostCommitPhase stage 4 of 7 with 5 MutationType ops
+upsert descriptor #104
+  ...
+     referencingDescriptorIds:
+     - 106
+  -  version: "5"
+  +  version: "6"
+upsert descriptor #105
+  ...
+     referencingDescriptorIds:
+     - 106
+  -  version: "5"
+  +  version: "6"
 upsert descriptor #106
   ...
          version: 4
@@ -287,7 +366,19 @@ begin transaction #7
 merge temporary indexes [3] into backfilled indexes [2] in table #106
 commit transaction #7
 begin transaction #8
-## PostCommitPhase stage 6 of 7 with 3 MutationType ops
+## PostCommitPhase stage 6 of 7 with 5 MutationType ops
+upsert descriptor #104
+  ...
+     referencingDescriptorIds:
+     - 106
+  -  version: "6"
+  +  version: "7"
+upsert descriptor #105
+  ...
+     referencingDescriptorIds:
+     - 106
+  -  version: "6"
+  +  version: "7"
 upsert descriptor #106
   ...
          version: 4
@@ -309,7 +400,31 @@ begin transaction #9
 validate forward indexes [2] in table #106
 commit transaction #9
 begin transaction #10
-## PostCommitNonRevertiblePhase stage 1 of 2 with 7 MutationType ops
+## PostCommitNonRevertiblePhase stage 1 of 2 with 9 MutationType ops
+upsert descriptor #104
+  ...
+         userName: root
+       jobId: "1"
+  -    revertible: true
+     enumMembers:
+     - logicalRepresentation: a
+  ...
+     referencingDescriptorIds:
+     - 106
+  -  version: "7"
+  +  version: "8"
+upsert descriptor #105
+  ...
+         userName: root
+       jobId: "1"
+  -    revertible: true
+     id: 105
+     kind: ALIAS
+  ...
+     referencingDescriptorIds:
+     - 106
+  -  version: "7"
+  +  version: "8"
 upsert descriptor #106
   ...
            statement: CREATE INDEX idx1 ON t (v) WHERE (v = 'a')
@@ -337,7 +452,7 @@ upsert descriptor #106
   +    - 1
   +    name: idx1
   +    partitioning: {}
-  +    predicate: (v = 'a')
+  +    predicate: v = b'@':::@100104
   +    sharded: {}
   +    storeColumnNames: []
   +    version: 4
@@ -362,7 +477,7 @@ upsert descriptor #106
   -      - 1
   -      name: idx1
   -      partitioning: {}
-  -      predicate: (v = 'a')
+  -      predicate: v = b'@':::@100104
   -      sharded: {}
   -      storeColumnNames: []
   -      version: 4
@@ -390,7 +505,36 @@ update progress of schema change job #1: "PostCommitNonRevertiblePhase stage 2 o
 set schema change job #1 to non-cancellable
 commit transaction #10
 begin transaction #11
-## PostCommitNonRevertiblePhase stage 2 of 2 with 4 MutationType ops
+## PostCommitNonRevertiblePhase stage 2 of 2 with 6 MutationType ops
+upsert descriptor #104
+   type:
+     arrayTypeId: 105
+  -  declarativeSchemaChangerState:
+  -    authorization:
+  -      userName: root
+  -    jobId: "1"
+     enumMembers:
+     - logicalRepresentation: a
+  ...
+     referencingDescriptorIds:
+     - 106
+  -  version: "8"
+  +  version: "9"
+upsert descriptor #105
+  ...
+       family: ArrayFamily
+       oid: 100105
+  -  declarativeSchemaChangerState:
+  -    authorization:
+  -      userName: root
+  -    jobId: "1"
+     id: 105
+     kind: ALIAS
+  ...
+     referencingDescriptorIds:
+     - 106
+  -  version: "8"
+  +  version: "9"
 upsert descriptor #106
   ...
      createAsOfTime: