sql: ALTER PK carries over comments from old to new primary index #114354

Xiang-Gu · 2023-11-13T17:14:42Z

Release note (bug fix): Previously, when session variable
use_declarative_schema_changer=off, ALTER PK would delete any comments
associated with the old primary index and with the old primary key
constraint. This is inconsistent with the behavior with
when use_declarative_schema_changer=on, which is the default setting,
where those comments would be carried over to the new primary index.
Furthermore, the old behavior also caused a bug that could prevent
command SHOW CREATE t from working (see #114081).

cockroach-teamcity · 2023-11-13T17:14:51Z

This change is

Xiang-Gu · 2023-11-13T19:30:29Z

We decided to not backport this change to 23.2 because in 23.2 the default setting uses the declarative schema changer and it won't run into the issue.

rafiss

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

-- commits line 6 at r1:
this should be Release note (bug fix)

-- commits line 7 at r1:
nit: "will" -> "would"

-- commits line 12 at r1:
"behavior is buggy" is too broad for a release note. Check out the guidelines at https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/186548364/Release+notes#Descriptions

This could instead say "The old behavior also caused a bug that could prevent the SHOW CREATE TABLE command from working."

pkg/sql/schema_changer.go line 1565 at r1 (raw file):

			}

			// If a primary index swap or any indexes are being dropped clean up any

it looks like this old behavior was initially added to fix a different bug: #76277

does your latest fix still work for that case?

pkg/sql/schema_changer.go line 1569 at r1 (raw file):

			// the comments associated with the old indexes to the new ones.
			if pkSwap := m.AsPrimaryKeySwap(); pkSwap != nil {
				oldIndexIDs := append([]descpb.IndexID{pkSwap.PrimaryKeySwapDesc().OldPrimaryIndexId}, pkSwap.PrimaryKeySwapDesc().OldIndexes...)

nit: these lines would be much less verbose if we declared a variable:

pkSwapDesc := pkSwap.PrimaryKeySwapDesc()

pkg/sql/schema_changer.go line 1576 at r1 (raw file):

						return err
					}
					newIndexDesc, err := catalog.MustFindIndexByID(scTable, newIndexIDs[i])

is there a contract somewhere that oldIndexIDs and newIndexIDs will always be the same length, and that the elements will be in an equivalent order to allow the swap below?

pkg/sql/logictest/testdata/logic_test/alter_primary_key line 1608 at r1 (raw file):


# Ensure `SHOW CREATE` does not break (issue #114081).
statement ok

would it be better for the assertion to actually show the result of this query? (i.e. use an onlyif config local-legacy-schema-changer directive, then check for the names)

chrisseto

LGTM! Just a couple comment nits

chrisseto · 2023-11-13T21:02:13Z

pkg/sql/logictest/testdata/logic_test/alter_primary_key

@@ -1602,6 +1604,10 @@ COMMENT ON INDEX public.pkey_comment@pkey IS 'idx';
 COMMENT ON CONSTRAINT i2 ON public.pkey_comment IS 'idx3';
 COMMENT ON CONSTRAINT pkey ON public.pkey_comment IS 'const'

+# Ensure `SHOW CREATE` does not break (issue #114081).


nit: Can you include a short description of #114081 ? If someone is reading through this code, having to navigate to a different resource to even get a hint of what's happening can be frustrating.

What would you think about:

# Ensure that comment references have been appropriately updated by running `SHOW CREATE` (See issue #114081 for details)

chrisseto · 2023-11-13T21:03:49Z

pkg/sql/show_create_clauses.go

@@ -45,7 +45,7 @@ type tableComments struct {
 }

 type comment struct {
-	subID   int
+	subID   int // the column, or index, or constraint ID that this comment is associated to.


nit (assuming that subID is short for SubjectID):

Suggested change

subID int // the column, or index, or constraint ID that this comment is associated to.

subID int // the column, index, or constraint ID that is the subject of this comment

chrisseto · 2023-11-13T21:10:18Z

pkg/sql/schema_changer.go

+			// If `m` is a primary index swap, which results in creations of a
+			// primary index and possibly new, rewritten secondary indexes, carry over
+			// the comments associated with the old indexes to the new ones.


small grammatical nit:

Suggested change

// If `m` is a primary index swap, which results in creations of a

// primary index and possibly new, rewritten secondary indexes, carry over

// the comments associated with the old indexes to the new ones.

// If `m` is a primary index swap, which results in the creation of a

// primary index and possibly new, rewritten, secondary indexes, carry over

// the comments associated with the old indexes to the new ones.

Fixes cockroachdb#114081 Release note (bug fix): Previously, when session variable `use_declarative_schema_changer=off`, ALTER PK would delete any comments associated with the old primary index and with the old primary key constraint. This is inconsistent with the behavior with when `use_declarative_schema_changer=on`, which is the default setting, where those comments would be carried over to the new primary index. Furthermore, the old behavior also caused a bug that could prevent command `SHOW CREATE t` from working (see cockroachdb#114081).

Xiang-Gu

Thanks for your feedback and I've addressed them all. @rafiss When you get a chance, can you give it another look? Thank you!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chrisseto and @rafiss)

-- commits line 6 at r1:

Previously, rafiss (Rafi Shamim) wrote…

this should be Release note (bug fix)

Done.

-- commits line 12 at r1:

Previously, rafiss (Rafi Shamim) wrote…

"behavior is buggy" is too broad for a release note. Check out the guidelines at https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/186548364/Release+notes#Descriptions

This could instead say "The old behavior also caused a bug that could prevent the SHOW CREATE TABLE command from working."

Done.

pkg/sql/schema_changer.go line 1565 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

it looks like this old behavior was initially added to fix a different bug: #76277

does your latest fix still work for that case?

yes, that fix was later overridden by this commit (55cd398) where we changed the behavior from "dropping comments" to "carrying over comments" in declarative schema changer. This PR is to change the legacy schema changer to match that behavior.

pkg/sql/schema_changer.go line 1567 at r1 (raw file):

Previously, chrisseto (Chris Seto) wrote…

small grammatical nit:

			// If `m` is a primary index swap, which results in the creation of a
			// primary index and possibly new, rewritten, secondary indexes, carry over
			// the comments associated with the old indexes to the new ones.

Done.

pkg/sql/schema_changer.go line 1576 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

is there a contract somewhere that oldIndexIDs and newIndexIDs will always be the same length, and that the elements will be in an equivalent order to allow the swap below?

It's a soft contract. So the intention is definitely for them to be in sync, as in comments here. The way we append to those two slices here ensures that, but I didn't see any checks. Do you think it'd be better if I add some guardrails here to ensure 1. their length is the same, and 2. they don't have duplicates, and if so, return an assertionError?

pkg/sql/logictest/testdata/logic_test/alter_primary_key line 1607 at r1 (raw file):

Previously, chrisseto (Chris Seto) wrote…

nit: Can you include a short description of #114081 ? If someone is reading through this code, having to navigate to a different resource to even get a hint of what's happening can be frustrating.

What would you think about:
# Ensure that comment references have been appropriately updated by running `SHOW CREATE` (See issue #114081 for details)

changed to an actual query to show the output of show create

pkg/sql/logictest/testdata/logic_test/alter_primary_key line 1608 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

would it be better for the assertion to actually show the result of this query? (i.e. use an onlyif config local-legacy-schema-changer directive, then check for the names)

I like this idea! It also makes it more obvious that the results are identical but in slightly different order. Done

rafiss

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

pkg/sql/schema_changer.go line 1576 at r1 (raw file):

Previously, Xiang-Gu (Xiang Gu) wrote…

It's a soft contract. So the intention is definitely for them to be in sync, as in comments here. The way we append to those two slices here ensures that, but I didn't see any checks. Do you think it'd be better if I add some guardrails here to ensure 1. their length is the same, and 2. they don't have duplicates, and if so, return an assertionError?

i think we can skip that, since i'd consider the comment you linked enough of a contract.

pkg/sql/logictest/testdata/logic_test/alter_primary_key line 1604 at r2 (raw file):

----
COMMENT ON INDEX public.pkey_comment@i2 IS 'idx2';
COMMENT ON INDEX public.pkey_comment@pkey IS 'idx';

interesting, i would have expected the new primary key to be called pkey_comment_pkey (like how the old schema changer names it) why did we pick this pkey name?

pkg/sql/logictest/testdata/logic_test/alter_primary_key line 1610 at r2 (raw file):

onlyif config local-legacy-schema-changer
query T
SELECT substring(create_statement, strpos(create_statement, 'COMMENT')) FROM [SHOW CREATE pkey_comment];

nice! if we add an ORDER BY to the results, could we could the same result in both configs?

Xiang-Gu

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chrisseto and @rafiss)

pkg/sql/schema_changer.go line 1576 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i think we can skip that, since i'd consider the comment you linked enough of a contract.

Done.

pkg/sql/logictest/testdata/logic_test/alter_primary_key line 1604 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

interesting, i would have expected the new primary key to be called pkey_comment_pkey (like how the old schema changer names it) why did we pick this pkey name?

It's an interesting debate -- do we think the new primary key should reuse the old primary key's name (dsc) or uses a new name (lsc). IMO, I'm inclined to DSC simply bc we are also carrying over the comments associated with the old primary index.

pkg/sql/logictest/testdata/logic_test/alter_primary_key line 1610 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nice! if we add an ORDER BY to the results, could we could the same result in both configs?

we could except that the name of the new primary key does not match, as illustrated above.

Xiang-Gu · 2023-11-16T21:47:06Z

TFTR!

bors r+

rafiss

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

pkg/sql/logictest/testdata/logic_test/alter_primary_key line 1604 at r2 (raw file):

Previously, Xiang-Gu (Xiang Gu) wrote…

It's an interesting debate -- do we think the new primary key should reuse the old primary key's name (dsc) or uses a new name (lsc). IMO, I'm inclined to DSC simply bc we are also carrying over the comments associated with the old primary index.

ah i see - if the name previously was explicitly named as pkey, then i agree with the new behavior too.

craig · 2023-11-16T23:11:45Z

Build succeeded:

Bazel Essential CI (Cockroach)

Xiang-Gu added the backport-23.2.x Flags PRs that need to be backported to 23.2. label Nov 13, 2023

Xiang-Gu marked this pull request as ready for review November 13, 2023 17:54

Xiang-Gu requested review from a team as code owners November 13, 2023 17:54

Xiang-Gu removed the backport-23.2.x Flags PRs that need to be backported to 23.2. label Nov 13, 2023

rafiss requested changes Nov 13, 2023

View reviewed changes

chrisseto approved these changes Nov 13, 2023

View reviewed changes

Xiang-Gu force-pushed the bug/show-create-breaks-legacy-alter-pk-with-comments branch from f599811 to 891b356 Compare November 14, 2023 16:11

Xiang-Gu commented Nov 14, 2023

View reviewed changes

Xiang-Gu mentioned this pull request Nov 14, 2023

sctest/comparator: modify line differently for LSC and DSC #114441

Merged

rafiss requested review from rafiss and chrisseto November 16, 2023 19:30

rafiss approved these changes Nov 16, 2023

View reviewed changes

Xiang-Gu commented Nov 16, 2023

View reviewed changes

rafiss reviewed Nov 16, 2023

View reviewed changes

craig bot merged commit aa93232 into cockroachdb:master Nov 16, 2023
8 checks passed

Xiang-Gu deleted the bug/show-create-breaks-legacy-alter-pk-with-comments branch November 17, 2023 18:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: ALTER PK carries over comments from old to new primary index #114354

sql: ALTER PK carries over comments from old to new primary index #114354

Xiang-Gu commented Nov 13, 2023 •

edited by rafiss

cockroach-teamcity commented Nov 13, 2023

Xiang-Gu commented Nov 13, 2023

rafiss left a comment

chrisseto left a comment

chrisseto Nov 13, 2023

chrisseto Nov 13, 2023

chrisseto Nov 13, 2023

Xiang-Gu left a comment

rafiss left a comment

Xiang-Gu left a comment

Xiang-Gu commented Nov 16, 2023

rafiss left a comment

craig bot commented Nov 16, 2023

	subID int // the column, or index, or constraint ID that this comment is associated to.
	subID int // the column, index, or constraint ID that is the subject of this comment

sql: ALTER PK carries over comments from old to new primary index #114354

sql: ALTER PK carries over comments from old to new primary index #114354

Conversation

Xiang-Gu commented Nov 13, 2023 • edited by rafiss

cockroach-teamcity commented Nov 13, 2023

Xiang-Gu commented Nov 13, 2023

rafiss left a comment

Choose a reason for hiding this comment

chrisseto left a comment

Choose a reason for hiding this comment

chrisseto Nov 13, 2023

Choose a reason for hiding this comment

chrisseto Nov 13, 2023

Choose a reason for hiding this comment

chrisseto Nov 13, 2023

Choose a reason for hiding this comment

Xiang-Gu left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

Xiang-Gu left a comment

Choose a reason for hiding this comment

Xiang-Gu commented Nov 16, 2023

rafiss left a comment

Choose a reason for hiding this comment

craig bot commented Nov 16, 2023

Xiang-Gu commented Nov 13, 2023 •

edited by rafiss