sql/schemachanger: implemented ALTER PRIMARY KEY for vanilla case #84514

Xiang-Gu · 2022-07-15T22:04:35Z

The PR implements ALTER PRIMARY KEY under the declarative schema
changer framework that handles the simplest, "vanilla" case like

CREATE TABLE t (i INT PRIMARY KEY, j INT NOT NULL)
ALTER TABLE t ALTER PRIMARY KEY USING COLUMNS (j)

This is the first of a series PRs where followup PRs will expand its
capabilities to be able to handle more complex cases, including

Allow the requested new primary key to be hash-sharded;
Consider the case where altering primary key requires us to
modify existing secondary indexes(see the legacy schema change
about in what cases we should rewrite)
Consider the case where the old primary index is on the implicitly
created rowid column, in which case we also need to drop that
column;
Consider partitioning and locality (I'm not sure what they are,
and why they play a role when ALTER PRIMARY KEY but I've seen
them in the old schema changer, so I assume we ought to do
something about them too here).
Support ALTER PRIMARY KEY with concurrent schema change
statements. E.g.
ALTER TABLE t ADD COLUMN k INT NOT NULL DEFAULT 30, ALTER PRIMARY KEY USING COLUMNS (j);

related: #83932
Release note: None

cockroach-teamcity · 2022-07-15T22:04:43Z

This change is

ajwerner · 2022-07-20T04:09:25Z

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

+	"github.com/cockroachdb/errors"
+)
+
+// The implementation of `ALTER PRIMARY KEY` will be broken into multiple PRs:


This plan belongs in an issue and not in the code.

ajwerner · 2022-07-20T04:09:50Z

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

+// 2. Consider the case where the new primary key is requested to be sharded.
+// 3. Consider the case where the old primary index is on the implicitly created `rowid` column,
+//    in which case we also need to drop that column;
+// 4. Consider the case where altering primary key requires us to modify existing secondary indexes


This step deserves to come before the rowid changes we don't currently support.

ajwerner · 2022-07-20T04:11:43Z

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

+	oldPrimaryIndexElem := mustRetrievePrimaryIndexElement(tableElems, tn.String())
+	oldPrimaryIndexNameElem := mustRetrievePrimaryIndexNameElem(tableElems, oldPrimaryIndexElem.IndexID, tn.String())
+
+	// Resolve and drop elements from the old primary index


How does this compose with adding or dropping columns? Let's look at this together tomorrow afternoon. In the meantime, can I encourage you to read the DROP COLUMN PR

Xiang-Gu

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

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go line 31 at r2 (raw file):

Previously, ajwerner wrote…

This plan belongs in an issue and not in the code.

done

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go line 36 at r2 (raw file):

Previously, ajwerner wrote…

This step deserves to come before the rowid changes we don't currently support.

ack

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go line 60 at r2 (raw file):

Previously, ajwerner wrote…

How does this compose with adding or dropping columns? Let's look at this together tomorrow afternoon. In the meantime, can I encourage you to read the DROP COLUMN PR

ack

postamar · 2022-07-26T00:11:10Z

pkg/sql/schemachanger/scbuild/builder_state.go

+		if constraintName.ConstraintID >= ret {
+			ret = constraintName.ConstraintID + 1
+		}
+	})


Drive-by reviewing: you should be able to walk through all the constraint IDs in the table by doing a little rel-magic:

nextID := catid.ConstraintID(1) b.QueryByID(table.TableID).ForEachElementStatus(func(_ scpb.Status, _ scpb.TargetStatus, e scpb.Element) { v, _ := screl.Schema.GetAttribute(screl.ConstraintID, e) if id, ok := v.(catid.ConstraintID); ok && id >= nextID { nextID = id + 1 } })

This should allow you to remove this method entirely, which would be nice.

Thanks! We have similar nextIndexID, nextColumnID method and I like having a nextConstraintID method as well (since it might be used more than once). But I do like and will change the implementation to what you suggested.

edit: in fact this method is never used in this PR (but will be in the next followup PR) so I delete it for now.

ajwerner

Getting close!

ajwerner · 2022-07-28T20:22:04Z

pkg/sql/sem/tree/select.go

+// ToIndexColumnDirection converts tree.Direction to catpb.IndexColumn_Direction.
+func (d Direction) ToIndexColumnDirection() catpb.IndexColumn_Direction {
+	switch d {
+	case DefaultDirection, Ascending:
+		return catpb.IndexColumn_ASC
+	case Descending:
+		return catpb.IndexColumn_DESC
+	default:
+		panic(fmt.Sprintf("invalid direction %s", d))
+	}
+}
+


I don't like this dependency. Can we put this somewhere else? I would love if we could fully separate the catalog protobufs from the ast where neither know about each other. It'll take some time, but this is moving backwards. Currently tree doesn't import any of the protobufs.

I'd expect the below rule to prevent this from passing.

disallowed_prefixes = [ "pkg/sql/catalog", "pkg/sql/catalog", ], ],

ajwerner · 2022-07-28T20:27:40Z

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

+	}
+
+	// TODO (xiang): This section contains all fall-back cases and need to
+	// 							 be removed to fully support `ALTER PRIMARY KEY`.


nit: bad indentation

ajwerner · 2022-07-28T20:27:57Z

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

+		//               thus a new primary index has already been created. We'd like
+		//               to support this use case one day
+		//               (e.g. `ALTER TABLE t ADD COLUMN ..., ALTER PRIMARY KEY ...;`).
+		//               Note that such scenarios should be caught above in
+		//               `fallBackIfConcurrentSchemaChange` and an unimplemented error
+		//               should be returned, so, here we panic with an programming error.


nit you don't need to wrap this like this, I think it's harder to read and annoying to maintain.

ajwerner · 2022-07-28T20:28:15Z

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

+		//               `fallBackIfConcurrentSchemaChange` and an unimplemented error
+		//               should be returned, so, here we panic with an programming error.
+		panic("programming error: new primary index has already existed.")
+	} else {


nit, unnest this, you don't need it nested given the panic.

ajwerner · 2022-07-28T20:30:25Z

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

+
+		colCurrentStatus, _, colElem := scpb.FindColumn(colElems)
+		if colElem == nil {
+			panic(fmt.Sprintf("programming error: resolving column %v does not give a "+


I'd rather you used an errors.AssertionFailedf error because it'll capture the stack trace. Maybe we need a helper panicf which combines panic and errors.AssertionFailedWithDepthf

The other advantage of the error constructors is they will properly handle redaction management for arguemnts. If you don't construct an error and instead format a string, the higher levels will redact the entire message.

ajwerner · 2022-07-28T20:35:17Z

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

+	return columnType
+}
+
+func mustRetrieveIndexElement(


One day before too long I'm going to come back and rework all of this to use rel and to be efficient. So many linear lookup loops.

ajwerner · 2022-07-28T20:36:31Z

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

+func dropOldPrimaryIndex(b BuildCtx, tableID catid.DescID, oldPrimaryIndexID catid.IndexID) {
+	oldPrimaryIndexName := mustRetrievePrimaryIndexNameElem(b, tableID, oldPrimaryIndexID).Name
+
+	b.ResolveIndex(tableID, tree.Name(oldPrimaryIndexName), ResolveParams{
+		IsExistenceOptional: false,
+		RequiredPrivilege:   privilege.CREATE,
+	}).ForEachElementStatus(func(_ scpb.Status, target scpb.TargetStatus, e scpb.Element) {
+		if target != scpb.ToAbsent {
+			b.Drop(e)
+		}
+	})
+}


Is this not handled by createNewPrimaryIndex?

ajwerner · 2022-07-28T20:36:41Z

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

+		}
+	}
+
+	// STORED columns (empty)


what's this about?

ajwerner · 2022-07-28T20:37:36Z

pkg/sql/schema_changer_test.go

+			if _, err := sqlDB.Exec(`
+SET use_declarative_schema_changer = off;
+ALTER TABLE t.test ALTER PRIMARY KEY USING COLUMNS (v2);
+SET use_declarative_schema_changer = on;`); err != nil {


move this change into the previous commit or change this to be the commit which enables the schema change. We don't want commits where tests fail. It makes bisecting things later hard.

ajwerner · 2022-07-28T20:37:51Z

pkg/sql/catalog/tabledesc/structured.go

@@ -111,7 +111,7 @@ func (desc *wrapper) GetParentSchemaID() descpb.ID {

 // IndexKeysPerRow implements the TableDescriptor interface.
 func (desc *wrapper) IndexKeysPerRow(idx catalog.Index) int {
-	if desc.PrimaryIndex.ID == idx.GetID() {
+	if desc.PrimaryIndex.ID == idx.GetID() || idx.GetEncodingType() == descpb.PrimaryIndexEncoding {


move this commit to be first

Xiang-Gu

@ajwerner I've addressed your comments and it's RFAL again.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @postamar)

pkg/sql/catalog/tabledesc/structured.go line 114 at r13 (raw file):

Previously, ajwerner wrote…

move this commit to be first

done

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go line 51 at r11 (raw file):

Previously, ajwerner wrote…

nit: bad indentation

done

Code quote:

	// TODO (xiang): This section contains all fall-back cases and need to
	// 							 be removed to fully support `ALTER PRIMARY KEY`.

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go line 67 at r11 (raw file):

Previously, ajwerner wrote…

nit you don't need to wrap this like this, I think it's harder to read and annoying to maintain.

done

Code quote:

Note that such scenarios should be caught above in

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go line 69 at r11 (raw file):

Previously, ajwerner wrote…

nit, unnest this, you don't need it nested given the panic.

done

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go line 173 at r11 (raw file):

Previously, ajwerner wrote…

The other advantage of the error constructors is they will properly handle redaction management for arguemnts. If you don't construct an error and instead format a string, the higher levels will redact the entire message.

done

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go line 221 at r11 (raw file):

Previously, ajwerner wrote…

nit: Use capital letters to start sentences.

done

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go line 253 at r11 (raw file):

Previously, ajwerner wrote…

I don't think this is good enough. Consider the case where we've had a previous statement in the same transaction but all of the elements in the statement phase have moved away from their initial status. Consider just checking if the elements are not in their terminal state.

you're right. Done!

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go line 340 at r11 (raw file):

Previously, ajwerner wrote…

same wrapping nit

done

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go line 408 at r11 (raw file):

Previously, ajwerner wrote…

One day before too long I'm going to come back and rework all of this to use rel and to be efficient. So many linear lookup loops.

totally supported! Thanks in advance!

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go line 458 at r11 (raw file):

Previously, ajwerner wrote…

Is this not handled by createNewPrimaryIndex?

you're right! This function is never used bc I forgot to delete it after I refactored the code to reuse createNewPrimaryIndex

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go line 614 at r11 (raw file):

Previously, ajwerner wrote…

what's this about?

I just want to leave a comment there to say there is no STORED columns in this case. I've removed this line.

pkg/sql/schema_changer_test.go line 2652 at r12 (raw file):

Previously, ajwerner wrote…

move this change into the previous commit or change this to be the commit which enables the schema change. We don't want commits where tests fail. It makes bisecting things later hard.

done. I've moved it to the last commit where I turn on ALTER PRIMARY KEY

pkg/sql/sem/tree/select.go line 758 at r10 (raw file):

Previously, ajwerner wrote…

I'd expect the below rule to prevent this from passing.
    disallowed_prefixes = [
        "pkg/sql/catalog",	        "pkg/sql/catalog",
    ],	    ],

I've moved this utility function to be in package scbuildstmt as one of many helpers there.

ajwerner

The rttanalysis failures are unexpected. You've got a couple more tests to fix.

Reviewed 2 of 6 files at r15, 1 of 2 files at r17.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @postamar, and @Xiang-Gu)

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go line 128 at r17 (raw file):

	b.EvalCtx().ClientNoticeSender.BufferClientNotice(b,
		pgnotice.Newf(
			"primary key changes are finalized asynchronously; "+

this is not true. If a test expects it, let's get rid of that test.

Xiang-Gu

I've fixed the CI and address the comments. RFAL

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @postamar)

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go line 128 at r17 (raw file):

Previously, ajwerner wrote…

this is not true. If a test expects it, let's get rid of that test.

done

ajwerner

Reviewed 2 of 8 files at r7, 1 of 15 files at r10, 2 of 11 files at r16, 1 of 12 files at r18, 2 of 2 files at r19, 3 of 4 files at r20, 3 of 20 files at r21, 2 of 2 files at r22.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @postamar, and @Xiang-Gu)

pkg/sql/logictest/testdata/logic_test/alter_primary_key line 7 at r22 (raw file):

INSERT INTO t VALUES (1, 2, 3, 4), (5, 6, 7, 8)

query T noticetrace

nit: change this to statement ok?

Code quote:

query T noticetrace
ALTER TABLE t ALTER PRIMARY KEY USING COLUMNS (y, z)

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go line 75 at r22 (raw file):

		// Get all KEY columns from t.Columns
		allColumnsName2IDMapping := getAllColumnsName2IDMapping(b, tbl.TableID)

nit: I don't think the use of 2 is clearer than To, can you rename all these?

Xiang-Gu

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner and @postamar)

pkg/sql/logictest/testdata/logic_test/alter_primary_key line 7 at r22 (raw file):

Previously, ajwerner wrote…

nit: change this to statement ok?

done

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go line 75 at r22 (raw file):

Previously, ajwerner wrote…

nit: I don't think the use of 2 is clearer than To, can you rename all these?

done

…or a row in an index We encounter an issue for this case ``` CREATE TABLE t (i INT PRIMARY KEY, j INT NOT NULL, FAMILY (i), FAMILY (j)); INSERT INTO t VALUES (23, 24); ALTER TABLE t ALTER PRIMARY KEY USING COLUMNS (j); ``` where the last `ALTER PRIMARY KEY` will result in duplicate entries in the new primary index, because the number of estimated keys for a row in this new primary index is incorrectly estimated to be 1, rather than 2 (the number of column families).

Previously, we issued a notice to customer for `ALTER PRIMARY KEY`, saying the altering primary key is done asynchronously and subsequent schema change might be rejected. This was no longer true (see the slace conversation https://cockroachlabs.slack.com/archives/C04U1BTF8/p1640124375172500 for some history. This PR removes this notice and modified the test that tests this notice.

The PR implements `ALTER PRIMARY KEY` under the declarative schema changer framework that handles the simplest, "vanilla" case like ``` CREATE TABLE t (i INT PRIMARY KEY, j INT NOT NULL) ALTER TABLE t ALTER PRIMARY KEY USING COLUMNS (j) ``` This is the first of a series PRs where followup PRs will expand its capabilities to be able to handle more complex cases, including 1. Allow the requested new primary key to be hash-sharded; 2. Consider the case where the old primary index is on the implicitly created `rowid` column, in which case we also need to drop that column; 3. Consider the case where altering primary key requires us to modify existing secondary indexes(see the legacy schema change about in what cases we should rewrite) 4. Consider partitioning and locality (I'm not sure what they are, and why they play a role when `ALTER PRIMARY KEY` but I've seen them in the old schema changer, so I assume we ought to do something about them too here). Release note: None

Release note: None

A few tests failed because they test/reply on specific behavior in the old schema changer, so we disabled them in this pr.

Xiang-Gu · 2022-08-09T14:30:10Z

bors r+

craig · 2022-08-09T16:04:43Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2022-08-09T18:32:51Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2022-08-09T19:11:50Z

Build failed:

Bazel Essential CI (Cockroach)

postamar · 2022-08-09T20:34:21Z

The merge failed due to a bunch of flakes, which ought to be fixed in the current bors batch. Retrying.

bors r+

craig · 2022-08-09T22:02:50Z

Build succeeded:

Bazel Essential CI (Cockroach)

Xiang-Gu marked this pull request as ready for review July 18, 2022 21:35

Xiang-Gu requested a review from a team July 18, 2022 21:35

ajwerner reviewed Jul 20, 2022

View reviewed changes

Xiang-Gu force-pushed the alter_primary_key_with_declarative_schema_changer_stage_1 branch from ea59185 to e8cb163 Compare July 20, 2022 18:45

Xiang-Gu commented Jul 20, 2022

View reviewed changes

Xiang-Gu force-pushed the alter_primary_key_with_declarative_schema_changer_stage_1 branch 5 times, most recently from 4d0080d to 6ffbfcf Compare July 25, 2022 14:39

postamar reviewed Jul 26, 2022

View reviewed changes

Xiang-Gu force-pushed the alter_primary_key_with_declarative_schema_changer_stage_1 branch 5 times, most recently from f36d289 to a3df350 Compare July 28, 2022 19:53

Xiang-Gu requested a review from a team as a code owner July 28, 2022 19:53

ajwerner reviewed Jul 28, 2022

View reviewed changes

Xiang-Gu force-pushed the alter_primary_key_with_declarative_schema_changer_stage_1 branch 2 times, most recently from 7491d62 to af18ceb Compare July 29, 2022 21:44

Xiang-Gu commented Jul 29, 2022

View reviewed changes

Xiang-Gu force-pushed the alter_primary_key_with_declarative_schema_changer_stage_1 branch from af18ceb to 4aa512c Compare August 1, 2022 14:07

ajwerner reviewed Aug 1, 2022

View reviewed changes

Xiang-Gu force-pushed the alter_primary_key_with_declarative_schema_changer_stage_1 branch 2 times, most recently from 911b970 to bdb3764 Compare August 2, 2022 22:12

Xiang-Gu commented Aug 5, 2022

View reviewed changes

ajwerner approved these changes Aug 8, 2022

View reviewed changes

Xiang-Gu force-pushed the alter_primary_key_with_declarative_schema_changer_stage_1 branch from bdb3764 to e2328e3 Compare August 8, 2022 16:01

Xiang-Gu commented Aug 8, 2022

View reviewed changes

Xiang-Gu added 5 commits August 8, 2022 17:35

sql/schemachanger: added tests for the ALTER PRIMARY KEY vanilla case

dfc168c

Release note: None

turn ALTER PRIMARY KEY on

8a25332

A few tests failed because they test/reply on specific behavior in the old schema changer, so we disabled them in this pr.

Xiang-Gu force-pushed the alter_primary_key_with_declarative_schema_changer_stage_1 branch from e2328e3 to 8a25332 Compare August 8, 2022 21:36

craig bot merged commit 8bcd0cd into cockroachdb:master Aug 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql/schemachanger: implemented ALTER PRIMARY KEY for vanilla case #84514

sql/schemachanger: implemented ALTER PRIMARY KEY for vanilla case #84514

Xiang-Gu commented Jul 15, 2022 •

edited

Loading

cockroach-teamcity commented Jul 15, 2022

ajwerner Jul 20, 2022

ajwerner Jul 20, 2022

ajwerner Jul 20, 2022

Xiang-Gu left a comment

postamar Jul 26, 2022

Xiang-Gu Jul 27, 2022

Xiang-Gu Jul 27, 2022

ajwerner left a comment

ajwerner Jul 28, 2022

ajwerner Jul 28, 2022

ajwerner Jul 28, 2022

ajwerner Jul 28, 2022

ajwerner Jul 28, 2022

ajwerner Jul 28, 2022

ajwerner Jul 28, 2022

ajwerner Jul 28, 2022

ajwerner Jul 28, 2022

ajwerner Jul 28, 2022

ajwerner Jul 28, 2022

ajwerner Jul 28, 2022

Xiang-Gu left a comment

ajwerner left a comment

Xiang-Gu left a comment

ajwerner left a comment

Xiang-Gu left a comment

Xiang-Gu commented Aug 9, 2022

craig bot commented Aug 9, 2022

craig bot commented Aug 9, 2022

craig bot commented Aug 9, 2022

postamar commented Aug 9, 2022

craig bot commented Aug 9, 2022

sql/schemachanger: implemented ALTER PRIMARY KEY for vanilla case #84514

sql/schemachanger: implemented ALTER PRIMARY KEY for vanilla case #84514

Conversation

Xiang-Gu commented Jul 15, 2022 • edited Loading

cockroach-teamcity commented Jul 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xiang-Gu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xiang-Gu left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

Xiang-Gu left a comment

Choose a reason for hiding this comment

ajwerner 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 Aug 9, 2022

craig bot commented Aug 9, 2022

craig bot commented Aug 9, 2022

craig bot commented Aug 9, 2022

postamar commented Aug 9, 2022

craig bot commented Aug 9, 2022

Xiang-Gu commented Jul 15, 2022 •

edited

Loading