sql: Add IF NOT EXISTS modifier to ALTER TABLE ... ADD CONSTRAINT #71257

postamar · 2021-10-07T01:28:00Z

Previously, it was not possible to execute ALTER TABLE ... ADD
CONSTRAINT IF NOT EXISTS ... statements. This commit adds this IF NOT
EXISTS modifier to the ADD CONSTRAINT command of the ALTER TABLE
statement. When the modifier is present and the defined constraint name
already exists in the table, the statement no-ops instead of erroring.

Note that this syntax is not supported in PostgreSQL, however PostgreSQL
has transactional schema changes and PL/pgSQL which make it possible to
achieve the same thing. CockroachDB has none of that yet.

Fixes #53007.

Release note (sql change): added ALTER TABLE ... ADD CONSTRAINT IF NOT
EXISTS.

cockroach-teamcity · 2021-10-07T01:29:06Z

This change is

postamar · 2021-10-07T16:56:53Z

@ekalinin I'm very sorry that I did not realize you already had a pull request up for this issue, #53095, and furthermore that you had recently intended to rebase it. It was right there in github, but I didn't notice until right now, my apologies.

I looked at your pull request and found it reassuring that we essentially took the same approach in implementing this. My only significant point of contention pertains to the handling the IF NOT EXISTS in the startExec method, pgcode. DuplicateObject is not the only error type that can be returned. Also, we'd probably have to check that the error is not marked as an assertion failure, and more generally (but perhaps that's just me) I'd rather not make too many assumptions on what and how the table descriptor validation works.

I also wasn't sure about changing the grammar for table_constraint, as that would allow IF NOT EXISTS modifiers in CREATE TABLE statements. These would have no effect, but it depends how strict we want to be. I don't have an opinion on this.

ekalinin · 2021-10-09T16:23:36Z

@ekalinin I'm very sorry that I did not realize you already had a pull request up for this issue, #53095, and furthermore that you had recently intended to rebase it. It was right there in github, but I didn't notice until right now, my apologies.

I looked at your pull request and found it reassuring that we essentially took the same approach in implementing this. My only significant point of contention pertains to the handling the IF NOT EXISTS in the startExec method, pgcode. DuplicateObject is not the only error type that can be returned. Also, we'd probably have to check that the error is not marked as an assertion failure, and more generally (but perhaps that's just me) I'd rather not make too many assumptions on what and how the table descriptor validation works.

I also wasn't sure about changing the grammar for table_constraint, as that would allow IF NOT EXISTS modifiers in CREATE TABLE statements. These would have no effect, but it depends how strict we want to be. I don't have an opinion on this.

Hey @postamar! I just closed my PR as i think your PR is better ;)
Thanks for notifying me.

postamar · 2021-10-14T14:14:25Z

I'm converting this PR back to draft mode to rebase it. Now that #70604 has been merged to master, I've been able to find a few more subtle bugs.

ajwerner · 2021-11-08T19:45:14Z

pkg/sql/parser/sql.y

@@ -2222,6 +2222,17 @@ alter_table_cmd:
      ValidationBehavior: $3.validationBehavior(),
    }
  }
+  // ALTER TABLE <name> ADD CONSTRAINT IF NOT EXISTS ...
+| ADD CONSTRAINT IF NOT EXISTS constraint_name constraint_elem opt_validate_behavior


What if instead of another parser case we added an opt_if_not_exist to the existing rule?

last time i tried something like that i got shift reduce'd to oblivion, but i didnt figure out why. you can try but i think that's why even in pg IF EXISTS / NOT EXISTS are always a separate clause.

The presence of IF NOT EXISTS makes the constraint_name mandatory instead of optional. There were several ways to deal with this awkwardness and because I don't have taste in these matters I picked the one which made for the smallest diff.

otan

sorry had this in pending for ages

otan · 2021-11-01T20:11:19Z

pkg/sql/alter_table.go

@@ -194,6 +194,12 @@ func (n *alterTableNode) startExec(params runParams) error {
 				return err
 			}
 		case *tree.AlterTableAddConstraint:
+			skip, err := validateConstraintNameIsNotUsed(n.tableDesc, t)


nit: pedantically:

if skip, err := validateConstraintNameIsNotUsed(n.tableDesc, t); err != nil { ... } else if skip { .... }

otan · 2021-11-10T21:03:37Z

pkg/sql/logictest/testdata/logic_test/alter_primary_key

@@ -578,7 +578,7 @@ t  CREATE TABLE public.t (
 statement ok
 CREATE INDEX i ON t (x);

-statement error pq: constraint with name i already exists
+statement error pgcode 42P07 an index named "i" already exists or is being dropped


i wonder if we should keep "constraint" here instead of "index" in the error message. seems more postgres-like

My rationale for this was that since I'm returning pgcode.DuplicateRelation I might as well reference a relation. I don't know what the right thing to do is here and I'm not familiar with postgres. Should I change the message to s/constraint/index only, or should I also change the pgcode to something else? I'll defer to you.

Hmm having looked into this further, it turns out that we sometimes return pgcode.DuplicateObject and a duplicate constraint name message, depending on which statements are executed.

For the sake of uniformity perhaps we should always return pgcode.DuplicateObject and not talk about indexes.

otan · 2021-11-10T21:05:00Z

pkg/sql/parser/sql.y

@@ -2222,6 +2222,17 @@ alter_table_cmd:
      ValidationBehavior: $3.validationBehavior(),
    }
  }
+  // ALTER TABLE <name> ADD CONSTRAINT IF NOT EXISTS ...
+| ADD CONSTRAINT IF NOT EXISTS constraint_name constraint_elem opt_validate_behavior


last time i tried something like that i got shift reduce'd to oblivion, but i didnt figure out why. you can try but i think that's why even in pg IF EXISTS / NOT EXISTS are always a separate clause.

pkg/sql/create_index.go

Previously, it was not possible to execute ALTER TABLE ... ADD CONSTRAINT IF NOT EXISTS ... statements. This commit adds this IF NOT EXISTS modifier to the ADD CONSTRAINT command of the ALTER TABLE statement. When the modifier is present and the defined constraint name already exists in the table, the statement no-ops instead of erroring. Note that this syntax is not supported in PostgreSQL, however PostgreSQL has transactional schema changes and PL/pgSQL which make it possible to achieve the same thing. CockroachDB has none of that yet. This commit also fixes some minor bugs or inconsistencies in the handling of duplicate index names. Fixes cockroachdb#53007. Release note (sql change): added ALTER TABLE ... ADD CONSTRAINT IF NOT EXISTS.

postamar · 2021-11-12T04:21:51Z

Thanks for the review!

bors r+

craig · 2021-11-12T05:15:31Z

Build succeeded:

GitHub CI (Cockroach)

postamar marked this pull request as ready for review October 7, 2021 16:44

postamar requested a review from a team October 7, 2021 16:44

ekalinin mentioned this pull request Oct 9, 2021

sql: add support for IF NOT EXISTS for ALTER TABLE ADD CONSTRAINT #53095

Closed

postamar marked this pull request as draft October 14, 2021 14:13

postamar force-pushed the add-constraint-if-not-exists branch 3 times, most recently from ca916db to d59d9ea Compare October 14, 2021 19:37

postamar marked this pull request as ready for review October 14, 2021 20:56

postamar requested a review from a team as a code owner October 14, 2021 20:56

postamar requested a review from otan October 26, 2021 17:23

postamar force-pushed the add-constraint-if-not-exists branch 2 times, most recently from 20bdaca to 49f2c5e Compare October 26, 2021 20:38

ajwerner reviewed Nov 8, 2021

View reviewed changes

otan reviewed Nov 10, 2021

View reviewed changes

postamar force-pushed the add-constraint-if-not-exists branch from 49f2c5e to b8bee29 Compare November 11, 2021 15:12

blathers-crl bot requested a review from otan November 11, 2021 15:12

postamar force-pushed the add-constraint-if-not-exists branch from b8bee29 to e89093f Compare November 11, 2021 15:13

otan approved these changes Nov 11, 2021

View reviewed changes

craig bot merged commit d16a755 into cockroachdb:master Nov 12, 2021

cockroach-teamcity mentioned this pull request Nov 12, 2021

sql: Add IF NOT EXISTS modifier to ALTER TABLE ... ADD CONSTRAINT cockroachdb/docs#12266

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: Add IF NOT EXISTS modifier to ALTER TABLE ... ADD CONSTRAINT #71257

sql: Add IF NOT EXISTS modifier to ALTER TABLE ... ADD CONSTRAINT #71257

postamar commented Oct 7, 2021

cockroach-teamcity commented Oct 7, 2021

postamar commented Oct 7, 2021

ekalinin commented Oct 9, 2021 •

edited

Loading

postamar commented Oct 14, 2021

ajwerner Nov 8, 2021

otan Nov 10, 2021

postamar Nov 11, 2021

otan left a comment

otan Nov 1, 2021

postamar Nov 11, 2021

otan Nov 10, 2021

postamar Nov 11, 2021

postamar Nov 11, 2021

otan Nov 10, 2021

postamar commented Nov 12, 2021

craig bot commented Nov 12, 2021

sql: Add IF NOT EXISTS modifier to ALTER TABLE ... ADD CONSTRAINT #71257

sql: Add IF NOT EXISTS modifier to ALTER TABLE ... ADD CONSTRAINT #71257

Conversation

postamar commented Oct 7, 2021

cockroach-teamcity commented Oct 7, 2021

postamar commented Oct 7, 2021

ekalinin commented Oct 9, 2021 • edited Loading

postamar commented Oct 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otan 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

postamar commented Nov 12, 2021

craig bot commented Nov 12, 2021

ekalinin commented Oct 9, 2021 •

edited

Loading