Merge #110216

110216: sql/schemachanger: create partial index with inserts can fail r=fqazi a=fqazi

Previously, the temporary index created during CREATE INDEX with partial indexes did not correctly copy over the predicate. If an insert and a CREATE INDEX (for partial indexes) were run concurrently, the CREATE INDEX could fail since the temporary index would contain extra rows. Eventually, validation would fail. This patch copies the embedded expression for partial indexes to address this.

Fixes: #110198

Release note (bug fix): CREATE INDEX for partial indexes could fail with "ERROR: duplicate key value violates unique constraint" if concurrent inserts happened simultaneously.

Co-authored-by: Faizan Qazi <faizan@cockroachlabs.com>

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go

@@ -684,18 +684,21 @@ func makeTempIndexSpec(src indexSpec) indexSpec {
 	}
 	newTempSpec := src.clone()
 	var srcIdx scpb.Index
+	var expr *scpb.Expression
 	isSecondary := false
 	if src.primary != nil {
 		srcIdx = newTempSpec.primary.Index
 	}
 	if src.secondary != nil {
 		srcIdx = newTempSpec.secondary.Index
+		expr = newTempSpec.secondary.EmbeddedExpr
 		isSecondary = true
 	}
 	tempID := srcIdx.TemporaryIndexID
 	newTempSpec.temporary = &scpb.TemporaryIndex{
 		Index:                    srcIdx,
 		IsUsingSecondaryEncoding: isSecondary,
+		Expr:                     expr,
 	}
 	newTempSpec.temporary.TemporaryIndexID = 0
 	newTempSpec.temporary.IndexID = tempID

pkg/sql/schemachanger/scpb/elements.proto

@@ -279,6 +279,7 @@ message SecondaryIndex {
 message TemporaryIndex {
   Index embedded_index = 1 [(gogoproto.nullable) = false, (gogoproto.embed) = true];
   bool is_using_secondary_encoding = 2;
+  Expression expr = 3 [(gogoproto.nullable) = true];
 }
 
 message SecondaryIndexPartial {

pkg/sql/schemachanger/scpb/uml/table.puml

@@ -370,6 +370,7 @@ object TemporaryIndex
 
 TemporaryIndex :  Index
 TemporaryIndex :  IsUsingSecondaryEncoding
+TemporaryIndex :  Expr
 
 object UniqueWithoutIndexConstraint
 

pkg/sql/schemachanger/scplan/internal/opgen/opgen_temporary_index.go

@@ -28,6 +28,17 @@ func init() {
 						IsSecondaryIndex: this.IsUsingSecondaryEncoding,
 					}
 				}),
+				emit(func(this *scpb.TemporaryIndex) *scop.SetAddedIndexPartialPredicate {
+					if this.Expr == nil {
+						return nil
+					}
+					return &scop.SetAddedIndexPartialPredicate{
+						TableID: this.TableID,
+						IndexID: this.IndexID,
+						Expr:    this.Expr.Expr,
+					}
+				}),
+
 				emit(func(this *scpb.TemporaryIndex, md *opGenContext) *scop.MaybeAddSplitForIndex {
 					// Avoid adding splits for tables without any data (i.e. newly created ones).
 					if checkIfDescriptorIsWithoutData(this.TableID, md) {

pkg/sql/schemachanger/testdata/end_to_end/create_index/create_index.definition

@@ -3,40 +3,46 @@ CREATE TYPE e AS ENUM('a', 'b', 'c');
 CREATE TABLE t (k INT PRIMARY KEY, v e NOT NULL);
 ----
 
-
+# Intentionally, insert one value for the partial index ('a')
+# (see below). Update one value so it will get added into
+# the partial index. The $stageKeyValue + 1 will not be in the
+# partial index. The delete remove the first insert which will
+# get added back again after. All these changes are propogated in
+# the temporary index.
 stage-exec phase=PostCommitPhase stage=:
 INSERT INTO t VALUES($stageKey, 'a');
 INSERT INTO t VALUES($stageKey + 1, 'b');
 INSERT INTO t VALUES($stageKey + 2, 'c');
 DELETE FROM t WHERE v = 'a' and k=$stageKey;
 INSERT INTO t VALUES($stageKey, 'a');
-UPDATE t SET v='a' WHERE k > 0;
+UPDATE t SET v='a' WHERE k % 2 = 0 
 ----
 
-# Each insert will be injected thrice per stage, so we should always,
-# see a count of 2.
+
+# The value 'a' is added twice per stage, see the rational above.
 stage-query phase=PostCommitPhase stage=:
-SELECT count(*)=$successfulStageCount*3 FROM t where v='a';
+SELECT count(*)=$successfulStageCount*2 FROM t where v='a';
 ----
 true
 
-
+# Similar to the sequence above in the PostCommitPhase.
 stage-exec phase=PostCommitNonRevertiblePhase stage=:
 INSERT INTO t VALUES($stageKey, 'a');
 INSERT INTO t VALUES($stageKey + 1, 'b');
 INSERT INTO t VALUES($stageKey + 2, 'c');
 DELETE FROM t WHERE v = 'a' and k=$stageKey;
 INSERT INTO t VALUES($stageKey, 'a');
-UPDATE t SET v='a' WHERE k > 0;
+UPDATE t SET v='a' WHERE k % 2 = 0;
 ----
 
-# Each insert will be injected twice per stage, so we should always,
-# see a count of 2.
+
+# The value 'a' is added twice per stage, see the rational above.
 stage-query phase=PostCommitNonRevertiblePhase stage=:
-SELECT count(*)=$successfulStageCount*3 FROM t where v='a';
+SELECT count(*)=$successfulStageCount*2 FROM t where v='a';
 ----
 true
 
+# Create a partial index on column V where the value is A
 test
 CREATE INDEX idx1 ON t (v) WHERE (v = 'a');
 ----

pkg/sql/schemachanger/testdata/end_to_end/create_index/create_index.explain

@@ -18,14 +18,15 @@ Schema change plan for CREATE INDEX ‹idx1› ON ‹defaultdb›.‹public›.
  │         │    ├── ABSENT → DELETE_ONLY   TemporaryIndex:{DescID: 106 (t), IndexID: 3, ConstraintID: 1, SourceIndexID: 1 (t_pkey)}
  │         │    ├── ABSENT → PUBLIC        IndexColumn:{DescID: 106 (t), ColumnID: 2 (v), IndexID: 3}
  │         │    └── ABSENT → PUBLIC        IndexColumn:{DescID: 106 (t), ColumnID: 1 (k), IndexID: 3}
- │         └── 9 Mutation operations
+ │         └── 10 Mutation operations
  │              ├── MakeAbsentIndexBackfilling {"IsSecondaryIndex":true}
  │              ├── SetAddedIndexPartialPredicate {"Expr":"v = b'@':::@1001...","IndexID":2,"TableID":106}
  │              ├── UpdateTableBackReferencesInTypes {"BackReferencedTableID":106}
  │              ├── AddColumnToIndex {"ColumnID":2,"IndexID":2,"TableID":106}
  │              ├── AddColumnToIndex {"ColumnID":1,"IndexID":2,"Kind":1,"TableID":106}
  │              ├── SetIndexName {"IndexID":2,"Name":"idx1","TableID":106}
  │              ├── MakeAbsentTempIndexDeleteOnly {"IsSecondaryIndex":true}
+ │              ├── SetAddedIndexPartialPredicate {"Expr":"v = b'@':::@1001...","IndexID":3,"TableID":106}
  │              ├── AddColumnToIndex {"ColumnID":2,"IndexID":3,"TableID":106}
  │              └── AddColumnToIndex {"ColumnID":1,"IndexID":3,"Kind":1,"TableID":106}
  ├── PreCommitPhase
@@ -53,7 +54,7 @@ Schema change plan for CREATE INDEX ‹idx1› ON ‹defaultdb›.‹public›.
  │         │    ├── ABSENT → DELETE_ONLY   TemporaryIndex:{DescID: 106 (t), IndexID: 3, ConstraintID: 1, SourceIndexID: 1 (t_pkey)}
  │         │    ├── ABSENT → PUBLIC        IndexColumn:{DescID: 106 (t), ColumnID: 2 (v), IndexID: 3}
  │         │    └── ABSENT → PUBLIC        IndexColumn:{DescID: 106 (t), ColumnID: 1 (k), IndexID: 3}
- │         └── 15 Mutation operations
+ │         └── 16 Mutation operations
  │              ├── MakeAbsentIndexBackfilling {"IsSecondaryIndex":true}
  │              ├── MaybeAddSplitForIndex {"IndexID":2,"TableID":106}
  │              ├── SetAddedIndexPartialPredicate {"Expr":"v = b'@':::@1001...","IndexID":2,"TableID":106}
@@ -62,6 +63,7 @@ Schema change plan for CREATE INDEX ‹idx1› ON ‹defaultdb›.‹public›.
  │              ├── AddColumnToIndex {"ColumnID":1,"IndexID":2,"Kind":1,"TableID":106}
  │              ├── SetIndexName {"IndexID":2,"Name":"idx1","TableID":106}
  │              ├── MakeAbsentTempIndexDeleteOnly {"IsSecondaryIndex":true}
+ │              ├── SetAddedIndexPartialPredicate {"Expr":"v = b'@':::@1001...","IndexID":3,"TableID":106}
  │              ├── MaybeAddSplitForIndex {"IndexID":3,"TableID":106}
  │              ├── AddColumnToIndex {"ColumnID":2,"IndexID":3,"TableID":106}
  │              ├── AddColumnToIndex {"ColumnID":1,"IndexID":3,"Kind":1,"TableID":106}

pkg/sql/schemachanger/testdata/end_to_end/create_index/create_index.side_effects

@@ -24,7 +24,7 @@ write *eventpb.CreateIndex to event log:
     tag: CREATE INDEX
     user: root
   tableName: defaultdb.public.t
-## StatementPhase stage 1 of 1 with 9 MutationType ops
+## StatementPhase stage 1 of 1 with 10 MutationType ops
 upsert descriptor #104
   ...
      referencingDescriptorIds:
@@ -84,6 +84,7 @@ upsert descriptor #106
   +      - 1
   +      name: crdb_internal_index_3_name_placeholder
   +      partitioning: {}
+  +      predicate: v = b'@':::@100104
   +      sharded: {}
   +      storeColumnNames: []
   +      useDeletePreservingEncoding: true
@@ -108,7 +109,7 @@ 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 15 MutationType ops
+## PreCommitPhase stage 2 of 2 with 16 MutationType ops
 upsert descriptor #104
    type:
      arrayTypeId: 105
@@ -224,6 +225,7 @@ upsert descriptor #106
   +      - 1
   +      name: crdb_internal_index_3_name_placeholder
   +      partitioning: {}
+  +      predicate: v = b'@':::@100104
   +      sharded: {}
   +      storeColumnNames: []
   +      useDeletePreservingEncoding: true
@@ -532,6 +534,7 @@ upsert descriptor #106
   -      - 1
   -      name: crdb_internal_index_3_name_placeholder
   -      partitioning: {}
+  -      predicate: v = b'@':::@100104
   -      sharded: {}
   -      storeColumnNames: []
   -      useDeletePreservingEncoding: true

pkg/sql/schemachanger/testdata/end_to_end/create_index_create_schema_separate_statements/create_index_create_schema_separate_statements.side_effects

@@ -23,7 +23,7 @@ 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
   ...
      id: 104
@@ -73,6 +73,7 @@ upsert descriptor #104
   +      - 1
   +      name: crdb_internal_index_3_name_placeholder
   +      partitioning: {}
+  +      predicate: i > 0:::INT8
   +      sharded: {}
   +      storeColumnNames: []
   +      unique: true
@@ -137,7 +138,7 @@ upsert descriptor #100
 ## 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 21 MutationType ops
+## PreCommitPhase stage 2 of 2 with 22 MutationType ops
 add schema namespace entry {100 0 sc} -> 105
 upsert descriptor #105
   -
@@ -275,6 +276,7 @@ upsert descriptor #104
   +      - 1
   +      name: crdb_internal_index_3_name_placeholder
   +      partitioning: {}
+  +      predicate: i > 0:::INT8
   +      sharded: {}
   +      storeColumnNames: []
   +      unique: true
@@ -570,6 +572,7 @@ upsert descriptor #104
   -      - 1
   -      name: crdb_internal_index_3_name_placeholder
   -      partitioning: {}
+  -      predicate: i > 0:::INT8
   -      sharded: {}
   -      storeColumnNames: []
   -      unique: true

pkg/sql/schemachanger/testdata/end_to_end/create_index_create_schema_separate_statements/create_index_create_schema_separate_statements__statement_1_of_2.explain

@@ -18,13 +18,14 @@ Schema change plan for CREATE UNIQUE INDEX ‹idx› ON ‹defaultdb›.‹publi
  │         │    ├── ABSENT → DELETE_ONLY   TemporaryIndex:{DescID: 104 (t), IndexID: 3, ConstraintID: 3, SourceIndexID: 1 (t_pkey)}
  │         │    ├── ABSENT → PUBLIC        IndexColumn:{DescID: 104 (t), ColumnID: 2 (j), IndexID: 3}
  │         │    └── ABSENT → PUBLIC        IndexColumn:{DescID: 104 (t), ColumnID: 1 (i), IndexID: 3}
- │         └── 8 Mutation operations
+ │         └── 9 Mutation operations
  │              ├── MakeAbsentIndexBackfilling {"IsSecondaryIndex":true}
  │              ├── SetAddedIndexPartialPredicate {"Expr":"i \u003e 0:::INT8","IndexID":2,"TableID":104}
  │              ├── AddColumnToIndex {"ColumnID":2,"IndexID":2,"TableID":104}
  │              ├── AddColumnToIndex {"ColumnID":1,"IndexID":2,"Kind":1,"TableID":104}
  │              ├── SetIndexName {"IndexID":2,"Name":"idx","TableID":104}
  │              ├── MakeAbsentTempIndexDeleteOnly {"IsSecondaryIndex":true}
+ │              ├── SetAddedIndexPartialPredicate {"Expr":"i \u003e 0:::INT8","IndexID":3,"TableID":104}
  │              ├── AddColumnToIndex {"ColumnID":2,"IndexID":3,"TableID":104}
  │              └── AddColumnToIndex {"ColumnID":1,"IndexID":3,"Kind":1,"TableID":104}
  ├── PreCommitPhase
@@ -52,14 +53,15 @@ Schema change plan for CREATE UNIQUE INDEX ‹idx› ON ‹defaultdb›.‹publi
  │         │    ├── ABSENT → DELETE_ONLY   TemporaryIndex:{DescID: 104 (t), IndexID: 3, ConstraintID: 3, SourceIndexID: 1 (t_pkey)}
  │         │    ├── ABSENT → PUBLIC        IndexColumn:{DescID: 104 (t), ColumnID: 2 (j), IndexID: 3}
  │         │    └── ABSENT → PUBLIC        IndexColumn:{DescID: 104 (t), ColumnID: 1 (i), IndexID: 3}
- │         └── 12 Mutation operations
+ │         └── 13 Mutation operations
  │              ├── MakeAbsentIndexBackfilling {"IsSecondaryIndex":true}
  │              ├── MaybeAddSplitForIndex {"IndexID":2,"TableID":104}
  │              ├── SetAddedIndexPartialPredicate {"Expr":"i \u003e 0:::INT8","IndexID":2,"TableID":104}
  │              ├── AddColumnToIndex {"ColumnID":2,"IndexID":2,"TableID":104}
  │              ├── AddColumnToIndex {"ColumnID":1,"IndexID":2,"Kind":1,"TableID":104}
  │              ├── SetIndexName {"IndexID":2,"Name":"idx","TableID":104}
  │              ├── MakeAbsentTempIndexDeleteOnly {"IsSecondaryIndex":true}
+ │              ├── SetAddedIndexPartialPredicate {"Expr":"i \u003e 0:::INT8","IndexID":3,"TableID":104}
  │              ├── MaybeAddSplitForIndex {"IndexID":3,"TableID":104}
  │              ├── AddColumnToIndex {"ColumnID":2,"IndexID":3,"TableID":104}
  │              ├── AddColumnToIndex {"ColumnID":1,"IndexID":3,"Kind":1,"TableID":104}

pkg/sql/schemachanger/testdata/end_to_end/create_index_create_schema_separate_statements/create_index_create_schema_separate_statements__statement_2_of_2.explain

@@ -62,11 +62,12 @@ Schema change plan for CREATE SCHEMA ‹defaultdb›.‹sc›; following CREATE
  │         │    ├── ABSENT → PUBLIC           IndexColumn:{DescID: 104 (t), ColumnID: 2 (j), IndexID: 3 (crdb_internal_index_3_name_placeholder)}
  │         │    ├── ABSENT → PUBLIC           IndexColumn:{DescID: 104 (t), ColumnID: 1 (i), IndexID: 3 (crdb_internal_index_3_name_placeholder)}
  │         │    └── ABSENT → DELETE_ONLY      TemporaryIndex:{DescID: 104 (t), IndexID: 3 (crdb_internal_index_3_name_placeholder), ConstraintID: 3, SourceIndexID: 1 (t_pkey)}
- │         └── 21 Mutation operations
+ │         └── 22 Mutation operations
  │              ├── MakeAbsentIndexBackfilling {"IsSecondaryIndex":true}
  │              ├── MaybeAddSplitForIndex {"IndexID":2,"TableID":104}
  │              ├── SetAddedIndexPartialPredicate {"Expr":"i \u003e 0:::INT8","IndexID":2,"TableID":104}
  │              ├── MakeAbsentTempIndexDeleteOnly {"IsSecondaryIndex":true}
+ │              ├── SetAddedIndexPartialPredicate {"Expr":"i \u003e 0:::INT8","IndexID":3,"TableID":104}
  │              ├── MaybeAddSplitForIndex {"IndexID":3,"TableID":104}
  │              ├── CreateSchemaDescriptor {"SchemaID":105}
  │              ├── SetNameInDescriptor {"DescriptorID":105,"Name":"sc"}