From 5e8569d65912e8129c599bf9acc7fb952ec6a6b9 Mon Sep 17 00:00:00 2001 From: Faizan Qazi Date: Thu, 7 Sep 2023 19:36:12 +0000 Subject: [PATCH] sql/schemachanger: create partial index with inserts can fail 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. --- .../scbuild/internal/scbuildstmt/helpers.go | 3 +++ pkg/sql/schemachanger/scpb/elements.proto | 1 + pkg/sql/schemachanger/scpb/uml/table.puml | 1 + .../internal/opgen/opgen_temporary_index.go | 11 ++++++++ .../create_index/create_index.definition | 26 ++++++++++++------- .../create_index/create_index.explain | 6 +++-- .../create_index/create_index.side_effects | 7 +++-- ...te_schema_separate_statements.side_effects | 7 +++-- ...arate_statements__statement_1_of_2.explain | 6 +++-- ...arate_statements__statement_2_of_2.explain | 3 ++- 10 files changed, 52 insertions(+), 19 deletions(-) diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go index fb6ec6a45278..0fe308f5673d 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go +++ b/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 diff --git a/pkg/sql/schemachanger/scpb/elements.proto b/pkg/sql/schemachanger/scpb/elements.proto index 1842fa0d9b56..5dde75cff402 100644 --- a/pkg/sql/schemachanger/scpb/elements.proto +++ b/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 { diff --git a/pkg/sql/schemachanger/scpb/uml/table.puml b/pkg/sql/schemachanger/scpb/uml/table.puml index 1089f7094f20..acce31e85cdf 100644 --- a/pkg/sql/schemachanger/scpb/uml/table.puml +++ b/pkg/sql/schemachanger/scpb/uml/table.puml @@ -370,6 +370,7 @@ object TemporaryIndex TemporaryIndex : Index TemporaryIndex : IsUsingSecondaryEncoding +TemporaryIndex : Expr object UniqueWithoutIndexConstraint diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_temporary_index.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_temporary_index.go index c33f4baa705f..b0c08967a2c6 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_temporary_index.go +++ b/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) { diff --git a/pkg/sql/schemachanger/testdata/end_to_end/create_index/create_index.definition b/pkg/sql/schemachanger/testdata/end_to_end/create_index/create_index.definition index 51b7c7b09725..39ecb6a04caa 100644 --- a/pkg/sql/schemachanger/testdata/end_to_end/create_index/create_index.definition +++ b/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'); ---- diff --git a/pkg/sql/schemachanger/testdata/end_to_end/create_index/create_index.explain b/pkg/sql/schemachanger/testdata/end_to_end/create_index/create_index.explain index dfaf08d4ade1..667a64d8ed19 100644 --- a/pkg/sql/schemachanger/testdata/end_to_end/create_index/create_index.explain +++ b/pkg/sql/schemachanger/testdata/end_to_end/create_index/create_index.explain @@ -18,7 +18,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} - │ └── 9 Mutation operations + │ └── 10 Mutation operations │ ├── MakeAbsentIndexBackfilling {"IsSecondaryIndex":true} │ ├── SetAddedIndexPartialPredicate {"Expr":"v = b'@':::@1001...","IndexID":2,"TableID":106} │ ├── UpdateTableBackReferencesInTypes {"BackReferencedTableID":106} @@ -26,6 +26,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} │ ├── 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} 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 f10d7f1134ac..44d4a119be0c 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 @@ -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 @@ -225,6 +226,7 @@ upsert descriptor #106 + - 1 + name: crdb_internal_index_3_name_placeholder + partitioning: {} + + predicate: v = b'@':::@100104 + sharded: {} + storeColumnNames: [] + useDeletePreservingEncoding: true @@ -534,6 +536,7 @@ upsert descriptor #106 - - 1 - name: crdb_internal_index_3_name_placeholder - partitioning: {} + - predicate: v = b'@':::@100104 - sharded: {} - storeColumnNames: [] - useDeletePreservingEncoding: true diff --git a/pkg/sql/schemachanger/testdata/end_to_end/create_index_create_schema_separate_statements/create_index_create_schema_separate_statements.side_effects b/pkg/sql/schemachanger/testdata/end_to_end/create_index_create_schema_separate_statements/create_index_create_schema_separate_statements.side_effects index 3ac79baea285..1a053cd5452a 100644 --- a/pkg/sql/schemachanger/testdata/end_to_end/create_index_create_schema_separate_statements/create_index_create_schema_separate_statements.side_effects +++ b/pkg/sql/schemachanger/testdata/end_to_end/create_index_create_schema_separate_statements/create_index_create_schema_separate_statements.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 8 MutationType ops +## StatementPhase stage 1 of 1 with 9 MutationType ops upsert descriptor #104 ... id: 104 @@ -74,6 +74,7 @@ upsert descriptor #104 + - 1 + name: crdb_internal_index_3_name_placeholder + partitioning: {} + + predicate: i > 0:::INT8 + sharded: {} + storeColumnNames: [] + unique: true @@ -138,7 +139,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 - @@ -277,6 +278,7 @@ upsert descriptor #104 + - 1 + name: crdb_internal_index_3_name_placeholder + partitioning: {} + + predicate: i > 0:::INT8 + sharded: {} + storeColumnNames: [] + unique: true @@ -573,6 +575,7 @@ upsert descriptor #104 - - 1 - name: crdb_internal_index_3_name_placeholder - partitioning: {} + - predicate: i > 0:::INT8 - sharded: {} - storeColumnNames: [] - unique: true diff --git a/pkg/sql/schemachanger/testdata/end_to_end/create_index_create_schema_separate_statements/create_index_create_schema_separate_statements__statement_1_of_2.explain b/pkg/sql/schemachanger/testdata/end_to_end/create_index_create_schema_separate_statements/create_index_create_schema_separate_statements__statement_1_of_2.explain index fe77258350b8..1be0a6f98ea4 100644 --- a/pkg/sql/schemachanger/testdata/end_to_end/create_index_create_schema_separate_statements/create_index_create_schema_separate_statements__statement_1_of_2.explain +++ b/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,7 +53,7 @@ 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} @@ -60,6 +61,7 @@ Schema change plan for CREATE UNIQUE INDEX ‹idx› ON ‹defaultdb›.‹publi │ ├── 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} diff --git a/pkg/sql/schemachanger/testdata/end_to_end/create_index_create_schema_separate_statements/create_index_create_schema_separate_statements__statement_2_of_2.explain b/pkg/sql/schemachanger/testdata/end_to_end/create_index_create_schema_separate_statements/create_index_create_schema_separate_statements__statement_2_of_2.explain index 3ab2be0d5bc8..f903d5913284 100644 --- a/pkg/sql/schemachanger/testdata/end_to_end/create_index_create_schema_separate_statements/create_index_create_schema_separate_statements__statement_2_of_2.explain +++ b/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"}