sql: rearchitecture ALTER TABLE RENAME, add support for renaming constraints #35091

knz · 2019-02-20T12:18:03Z

Fixes #32555. For TypeORM compat, see discussion on #22298.

Release note (sql change): This patch changes RENAME COLUMN to become
a "table command", which can be used alongside other table commands in
a single ALTER TABLE statement. This makes it possible to e.g. atomically
add a computed column based on an existing column, and rename the
columns so that the computed column "replaces" the original column.

Release note (sql change): CockroachDB now supports ALTER TABLE RENAME
CONSTRAINT for compatibility with PostgreSQL. This feature is limited
to named constraints, where the name of the constraints is preserved
in the table metadata. This currently includes CHECK, UNIQUE and
FOREIGN KEY constraints, and does not include other
constraints (DEFAULT, NULL etc) otherwise supported by PostgreSQL. For
UNIQUE constraint, only supporting indexes that are not depended on by
views can be renamed.

cockroach-teamcity · 2019-02-20T12:18:15Z

This change is

knz · 2019-02-25T15:34:38Z

ping

dt · 2019-02-25T19:52:59Z

just want to make sure you're sync'ed up with @lucy-zhang who's actively working on some constraint-related schema mgmt stuff right now

thoszhang

I'll review this more thoroughly later, but I have one question for now.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bobvawter, @BramGruneir, @dt, @knz, and @lucy-zhang)

pkg/sql/sqlbase/structured.go, line 2110 at r1 (raw file):

// RenameConstraint renames a constraint.
func (desc *MutableTableDescriptor) RenameConstraint(

I haven't tested this myself yet, but what happens if a constraint is renamed while it's being added? We now enqueue a mutation to validate check constraints when they're added, and the mutation only stores the name of the check constraint (since they don't have IDs, which is unfortunate), so when the validation runs, I think it will fail if the constraint has been renamed in the meantime.

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bobvawter, @BramGruneir, @dt, and @lucy-zhang)

pkg/sql/sqlbase/structured.go, line 2110 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

I haven't tested this myself yet, but what happens if a constraint is renamed while it's being added? We now enqueue a mutation to validate check constraints when they're added, and the mutation only stores the name of the check constraint (since they don't have IDs, which is unfortunate), so when the validation runs, I think it will fail if the constraint has been renamed in the meantime.

I don't have an answer for this. You're right it may break. I can add a TODO, file an issue, and add an extra error check in the constraint validation code in case "the name does not exist any more". I think the case is sufficiently rare that an error in that case is ok for now. What do you think?

(Alternatively if you have an idea of a solution, I'll take it)

thoszhang

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bobvawter, @BramGruneir, @dt, and @knz)

pkg/sql/sqlbase/structured.go, line 2110 at r1 (raw file):

Previously, knz (kena) wrote…

I don't have an answer for this. You're right it may break. I can add a TODO, file an issue, and add an extra error check in the constraint validation code in case "the name does not exist any more". I think the case is sufficiently rare that an error in that case is ok for now. What do you think?

(Alternatively if you have an idea of a solution, I'll take it)

Could we disallow renaming constraints that are being added? We currently disallow deleting constraints being added: https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/alter_table.go#L466-L468

thoszhang

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bobvawter, @BramGruneir, @dt, and @knz)

pkg/sql/sqlbase/structured.go, line 2110 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

Could we disallow renaming constraints that are being added? We currently disallow deleting constraints being added: https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/alter_table.go#L466-L468

This is just for check constraints. I don't think FKs (and other constraints) have this problem, since I haven't made changes to FKs yet.

bobvawter

I'll second Lucy's point about dealing with renames in the face of a long-running schema change. I think it's certainly reasonable to kick back an error message to the user saying that the proposed change conflicts with a pending schema mutation and to retry later.

Reviewed 14 of 14 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @BramGruneir, @dt, and @knz)

pkg/sql/alter_table.go, line 609 at r1 (raw file):

		case *tree.AlterTableRenameColumn:
			descChanged, err := params.p.renameColumn(params.ctx, n.tableDesc, &t.Column, &t.NewName)

Does this need a privilege check like the constraint rename below, or is that handled elsewhere in the more general alter table flow?

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bobvawter, @BramGruneir, @dt, and @lucy-zhang)

pkg/sql/alter_table.go, line 609 at r1 (raw file):

Previously, bobvawter (Bob Vawter) wrote…

Does this need a privilege check like the constraint rename below, or is that handled elsewhere in the more general alter table flow?

The privilege check is made inside renameColumn().

pkg/sql/sqlbase/structured.go, line 2110 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

This is just for check constraints. I don't think FKs (and other constraints) have this problem, since I haven't made changes to FKs yet.

Ok thanks I have added the check. Can you hand-hold me as to where I can put a test for this? Thanks

thoszhang

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bobvawter, @BramGruneir, @dt, and @knz)

pkg/sql/sqlbase/structured.go, line 2110 at r1 (raw file):

Previously, knz (kena) wrote…

Ok thanks I have added the check. Can you hand-hold me as to where I can put a test for this? Thanks

We have some logic tests for the case where the disallowed schema change happens in the same transaction as creating the constraint: https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/logictest/testdata/logic_test/schema_change_in_txn#L844-L895

There is also a similar test in schema_changer_test.go where we test that a column can't be dropped if there is a check constraint being added that references it: https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/schema_changer_test.go#L1244

…traints Release note (sql change): This patch changes RENAME COLUMN to become a "table command", which can be used alongside other table commands in a single ALTER TABLE statement. This makes it possible to e.g. atomically add a computed column based on an existing column, and rename the columns so that the computed column "replaces" the original column. Release note (sql change): CockroachDB now supports ALTER TABLE RENAME CONSTRAINT. Only indexes that are not depended on by views can be renamed.

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bobvawter, @BramGruneir, @dt, and @lucy-zhang)

pkg/sql/sqlbase/structured.go, line 2110 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

We have some logic tests for the case where the disallowed schema change happens in the same transaction as creating the constraint: https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/logictest/testdata/logic_test/schema_change_in_txn#L844-L895

There is also a similar test in schema_changer_test.go where we test that a column can't be dropped if there is a check constraint being added that references it: https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/schema_changer_test.go#L1244

Added the test. Thanks for your guidance!

knz · 2019-03-07T19:24:38Z

bors r+

35091: sql: rearchitecture ALTER TABLE RENAME, add support for renaming constraints r=knz a=knz Fixes #32555. For TypeORM compat, see discussion on #22298. Release note (sql change): This patch changes RENAME COLUMN to become a "table command", which can be used alongside other table commands in a single ALTER TABLE statement. This makes it possible to e.g. atomically add a computed column based on an existing column, and rename the columns so that the computed column "replaces" the original column. Release note (sql change): CockroachDB now supports ALTER TABLE RENAME CONSTRAINT for compatibility with PostgreSQL. This feature is limited to *named* constraints, where the name of the constraints is preserved in the table metadata. This currently includes CHECK, UNIQUE and FOREIGN KEY constraints, and does not include other constraints (DEFAULT, NULL etc) otherwise supported by PostgreSQL. For UNIQUE constraint, only supporting indexes that are not depended on by views can be renamed. 35121: sql: add support for pg_catalog.{current_setting,set_config} r=knz a=knz Fixes #35108. Needed for Flowable compatibility. Release note (sql change): The SQL built-in functions `pg_catalog.current_setting()` and `pg_catalog.set_config()` are now supported for compatibility with PostgreSQL. Note that only session-scoped configuration changes remain supported (`set_config(_, _, false)`). 35359: roachtest: backup: use AOST time slightly in the past r=mjibson a=mjibson Avoids problems where it's sometimes in the future according to the AOST logic. Fixes #34817 Release note: None 35371: sql: ensure column constraints are validated after SET for UPSERT r=knz a=knz Fixes #35040. Release note (bug fix): CockroachDB now properly applies column width and nullability constraints on the result of conflict resolution in UPSERT and INSERT ON CONFLICT. Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com> Co-authored-by: Matt Jibson <matt.jibson@gmail.com>

craig · 2019-03-07T19:43:11Z

Build succeeded

GitHub CI (Cockroach)

dt · 2019-03-07T20:32:50Z

pkg/sql/logictest/testdata/logic_test/rename_column

+
+query T
+SELECT species FROM users
+----


knz added this to Triage in (DEPRECATED) SQL Front-end, Lang & Semantics via automation Feb 20, 2019

knz requested review from bobvawter, BramGruneir and a team February 20, 2019 12:18

knz force-pushed the 20190220-rename branch from b6811fd to 672b5af Compare February 20, 2019 12:27

knz mentioned this pull request Feb 20, 2019

sql: support ALTER TABLE ... RENAME CONSTRAINT #32555

Closed

knz force-pushed the 20190220-rename branch from 672b5af to d005dee Compare February 25, 2019 15:34

knz requested review from a team and dt February 25, 2019 15:34

dt requested a review from thoszhang February 25, 2019 15:37

thoszhang reviewed Feb 25, 2019

View reviewed changes

knz commented Feb 25, 2019

View reviewed changes

thoszhang reviewed Feb 25, 2019

View reviewed changes

bobvawter approved these changes Feb 26, 2019

View reviewed changes

knz force-pushed the 20190220-rename branch from d005dee to 64ea40b Compare February 26, 2019 20:51

knz commented Feb 26, 2019

View reviewed changes

knz force-pushed the 20190220-rename branch 2 times, most recently from 6aefcaa to ba582d0 Compare February 26, 2019 21:38

thoszhang reviewed Feb 27, 2019

View reviewed changes

knz force-pushed the 20190220-rename branch from ba582d0 to 11a7d81 Compare March 7, 2019 19:24

knz commented Mar 7, 2019

View reviewed changes

craig bot merged commit 11a7d81 into cockroachdb:master Mar 7, 2019

knz deleted the 20190220-rename branch March 7, 2019 19:57

knz added the docs-todo label Mar 7, 2019

dt reviewed Mar 7, 2019

View reviewed changes

pkg/sql/logictest/testdata/logic_test/rename_column

query T

SELECT species FROM users

----

Copy link

Member

dt Mar 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rowsort

dt mentioned this pull request Mar 7, 2019

tscache: remove a couple more unit test logf calls #35478

Merged

This was referenced Mar 18, 2019

sql: rearchitecture ALTER TABLE RENAME, add support for renaming constraints cockroachdb/docs#4519

Closed

sql: rearchitecture ALTER TABLE RENAME, add support for renaming constraints cockroachdb/docs#4520

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: rearchitecture ALTER TABLE RENAME, add support for renaming constraints #35091

sql: rearchitecture ALTER TABLE RENAME, add support for renaming constraints #35091

knz commented Feb 20, 2019

cockroach-teamcity commented Feb 20, 2019

knz commented Feb 25, 2019

dt commented Feb 25, 2019 •

edited

thoszhang left a comment

knz left a comment

thoszhang left a comment

thoszhang left a comment

bobvawter left a comment

knz left a comment

thoszhang left a comment

knz left a comment

knz commented Mar 7, 2019

craig bot commented Mar 7, 2019

dt Mar 7, 2019

sql: rearchitecture ALTER TABLE RENAME, add support for renaming constraints #35091

sql: rearchitecture ALTER TABLE RENAME, add support for renaming constraints #35091

Conversation

knz commented Feb 20, 2019

cockroach-teamcity commented Feb 20, 2019

knz commented Feb 25, 2019

dt commented Feb 25, 2019 • edited

thoszhang left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

thoszhang left a comment

Choose a reason for hiding this comment

thoszhang left a comment

Choose a reason for hiding this comment

bobvawter left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

thoszhang left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

knz commented Mar 7, 2019

craig bot commented Mar 7, 2019

Build succeeded

dt Mar 7, 2019

Choose a reason for hiding this comment

dt commented Feb 25, 2019 •

edited