sql/schemachanger: create partial index with inserts can fail #110216

fqazi · 2023-09-07T20:48:35Z

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.

cockroach-teamcity · 2023-09-07T20:48:43Z

This change is

ecwall · 2023-09-08T13:28:55Z

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

 		isSecondary = true
 	}
 	tempID := srcIdx.TemporaryIndexID
 	newTempSpec.temporary = &scpb.TemporaryIndex{
 		Index:                    srcIdx,
 		IsUsingSecondaryEncoding: isSecondary,
+		EmbeddedExpr:             expr,


Can the indexSpec be marked as temporary instead of copying parts of the top level spec into the temporary section?

fqazi

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ecwall)

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go line 701 at r1 (raw file):

Previously, ecwall (Evan Wall) wrote…

Can the indexSpec be marked as temporary instead of copying parts of the top level spec into the temporary section?

So, I think you are suggesting that an indexSpec for a temporary is a copy with just a flag flipped. I think that works for some things, but then there are a bunch of rewrites that happen below to assign IDs. Also, the apply I think would need to generate a TemporryIndex element from the original/primary secondary index elements.

I think in an ideal world SecondaryIndex == TemporaryIndex in terms of content minus the encoding indicator. But we also can't poke at scpb since that's written to descriptors :(

Xiang-Gu · 2023-09-11T15:46:09Z

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 embedded_expr = 3 [(gogoproto.nullable) = true];


nit: add some comments around this new field
nit: if you really intend this to be embedded (via (gogoproto.embed) = true), then consider changing the name.

Xiang-Gu · 2023-09-11T15:47:18Z

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

 # Each insert will be injected thrice per stage, so we should always,
 # see a count of 2.


can we add or modify the comments to help readers understand why a count of 2?

chrisseto · 2023-09-11T17:27:51Z

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

@@ -10,13 +10,14 @@ 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 


Can you add a note as to why % 2 is being used in favor of > 0?

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: cockroachdb#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.

fqazi

Reviewed 2 of 5 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chrisseto, @ecwall, and @Xiang-Gu)

pkg/sql/schemachanger/testdata/end_to_end/create_index/create_index.definition line 13 at r1 (raw file):

Previously, chrisseto (Chris Seto) wrote…

Can you add a note as to why % 2 is being used in favor of > 0?

Done.

pkg/sql/schemachanger/testdata/end_to_end/create_index/create_index.definition line 18 at r1 (raw file):

Previously, Xiang-Gu (Xiang Gu) wrote…

can we add or modify the comments to help readers understand why a count of 2?

Done.

chrisseto

LGTM!

rafiss

nice finds!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chrisseto, @ecwall, and @Xiang-Gu)

fqazi · 2023-09-13T16:04:26Z

@Xiang-Gu @chrisseto @rafiss @ecwall TFTR!

bors r+

craig · 2023-09-13T17:15:07Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl · 2023-09-13T17:15:16Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from 5e8569d to blathers/backport-release-23.1-110216: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

daniel-crlabs · 2024-02-26T14:18:22Z

@fqazi @rafiss Any idea when this fix will be released? We have customers running into this issue duplicate key value violates unique constraint and having to disable it with what's shown below (customer is on 23.1.11)

SET use_declarative_schema_changer = off;

Once the customers do the above they are able to create the index. Are there any negative consequences to disabling the declarative schema changer in this scenario (creating indexes)? Can they safely move forward with SET use_declarative_schema_changer = off for any session where they this king of index? Example below:

CREATE INDEX index_name_here 
ON table_name_here (bucketnum, keyname) 
WHERE (validstate = 3) AND (versioned = true);

fqazi · 2024-02-26T14:31:47Z

@daniel-crlabs The fix is already merged into 23.1.12 they just need to move up: #110584. Ideally they should only disable the declarative schema changer for CREATE INDEX (especially if they have larger data sets).

daniel-crlabs · 2024-02-26T14:37:14Z

Thank you, I'll let them know

fqazi requested a review from a team as a code owner September 7, 2023 20:48

fqazi force-pushed the partialIndexInsertBug branch from d21a2ee to 62c146b Compare September 7, 2023 21:06

fqazi added branch-release-23.1 Used to mark GA and release blockers and technical advisories for 23.1 backport-23.1.x Flags PRs that need to be backported to 23.1 and removed branch-release-23.1 Used to mark GA and release blockers and technical advisories for 23.1 labels Sep 8, 2023

ecwall reviewed Sep 8, 2023

View reviewed changes

fqazi commented Sep 8, 2023

View reviewed changes

Xiang-Gu reviewed Sep 11, 2023

View reviewed changes

chrisseto reviewed Sep 11, 2023

View reviewed changes

fqazi force-pushed the partialIndexInsertBug branch from 62c146b to 5e8569d Compare September 12, 2023 20:47

fqazi commented Sep 12, 2023

View reviewed changes

fqazi requested review from ecwall, Xiang-Gu and chrisseto September 13, 2023 13:15

Xiang-Gu approved these changes Sep 13, 2023

View reviewed changes

chrisseto approved these changes Sep 13, 2023

View reviewed changes

rafiss approved these changes Sep 13, 2023

View reviewed changes

craig bot merged commit 8316976 into cockroachdb:master Sep 13, 2023
8 checks passed

rafiss mentioned this pull request Oct 2, 2023

schema: add insert/update/delete DML injection test #111623

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql/schemachanger: create partial index with inserts can fail #110216

sql/schemachanger: create partial index with inserts can fail #110216

fqazi commented Sep 7, 2023

cockroach-teamcity commented Sep 7, 2023

ecwall Sep 8, 2023

fqazi left a comment

Xiang-Gu Sep 11, 2023 •

edited

Xiang-Gu Sep 11, 2023

chrisseto Sep 11, 2023

fqazi left a comment

chrisseto left a comment

rafiss left a comment

fqazi commented Sep 13, 2023

craig bot commented Sep 13, 2023

blathers-crl bot commented Sep 13, 2023

daniel-crlabs commented Feb 26, 2024

fqazi commented Feb 26, 2024

daniel-crlabs commented Feb 26, 2024

		# Each insert will be injected thrice per stage, so we should always,
		# see a count of 2.

sql/schemachanger: create partial index with inserts can fail #110216

sql/schemachanger: create partial index with inserts can fail #110216

Conversation

fqazi commented Sep 7, 2023

cockroach-teamcity commented Sep 7, 2023

ecwall Sep 8, 2023

Choose a reason for hiding this comment

fqazi left a comment

Choose a reason for hiding this comment

Xiang-Gu Sep 11, 2023 • edited

Choose a reason for hiding this comment

Xiang-Gu Sep 11, 2023

Choose a reason for hiding this comment

chrisseto Sep 11, 2023

Choose a reason for hiding this comment

fqazi left a comment

Choose a reason for hiding this comment

chrisseto left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

fqazi commented Sep 13, 2023

craig bot commented Sep 13, 2023

blathers-crl bot commented Sep 13, 2023

daniel-crlabs commented Feb 26, 2024

fqazi commented Feb 26, 2024

daniel-crlabs commented Feb 26, 2024

Xiang-Gu Sep 11, 2023 •

edited