Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: ALTER TABLE ... ADD COLUMN ... PRIMARY KEY doesn't work correctly #81449

Closed
postamar opened this issue May 18, 2022 · 4 comments
Closed
Labels
A-schema-changes C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@postamar
Copy link
Contributor

postamar commented May 18, 2022

To repro:

CREATE TABLE t (a INT PRIMARY KEY);
ALTER TABLE t ADD COLUMN b INT PRIMARY KEY;

Expected: the primary key is b an error is returned (in pg: multiple primary keys for table "t" are not allowed)
Actual: success, and a subsequent SHOW CREATE TABLE t yields

  table_name |              create_statement
-------------+---------------------------------------------
  t          | CREATE TABLE public.t (
             |     a INT8 NOT NULL,
             |     b INT8 NOT NULL,
             |     CONSTRAINT t_pkey PRIMARY KEY (a ASC),
             |     UNIQUE INDEX t_b_key (b ASC)
             | )
(1 row)

Jira issue: CRDB-15178

@postamar postamar added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label May 18, 2022
@blathers-crl blathers-crl bot added the T-sql-schema-deprecated Use T-sql-foundations instead label May 18, 2022
@otan
Copy link
Contributor

otan commented May 18, 2022

seems more like we want an error for the second statement right?

e.g. PG

otan=# CREATE TABLE t (a INT PRIMARY KEY);
ALTER TABLE t ADD COLUMN b INT PRIMARY KEY;
CREATE TABLE
ERROR:  multiple primary keys for table "t" are not allowed

@postamar
Copy link
Contributor Author

You're most likely correct, I didn't cross-check with postgres, sorry.

@postamar postamar changed the title sql: ALTER TABLE ... ADD COLUMN ... PRIMARY KEY doesn't work sql: ALTER TABLE ... ADD COLUMN ... PRIMARY KEY doesn't work correctly May 19, 2022
@jlinder jlinder added sync-me and removed sync-me labels May 20, 2022
@ajwerner
Copy link
Contributor

ajwerner commented Jun 7, 2022

I agree we should reject CREATE TABLE t (a INT PRIMARY KEY); ALTER TABLE t ADD COLUMN b INT PRIMARY KEY;, but what to do in the case that the primary key is the implicit rowid primary key is less clear. Consider the following which is valid in postgres CREATE TABLE t (a INT); ALTER TABLE t ADD COLUMN b INT PRIMARY KEY;

@otan
Copy link
Contributor

otan commented Jun 7, 2022

CREATE TABLE t (a INT PRIMARY KEY); ALTER TABLE t ADD COLUMN b INT PRIMARY KEY;

feels like we should definitely allow it if rowid is the hidden default PK... at least, it should be 100% true if it's the same txn - i think that is the case today

[email protected]:26257/defaultdb>  CREATE TABLE t (a INT PRIMARY KEY); ALTER TABLE t ADD COLUMN b INT PRIMARY KEY;
CREATE TABLE
ALTER TABLE

Note: timings for multiple statements on a single line are not supported. See https://go.crdb.dev/issue-v/48180/v22.2.

[email protected]:26257/defaultdb> show create table t;
  table_name |              create_statement
-------------+---------------------------------------------
  t          | CREATE TABLE public.t (
             |     a INT8 NOT NULL,
             |     b INT8 NOT NULL,
             |     CONSTRAINT t_pkey PRIMARY KEY (a ASC),
             |     UNIQUE INDEX t_b_key (b ASC)
             | )
(1 row)


Time: 25ms total (execution 24ms / network 1ms)

that said, it is totally broken in the new schema changer:

[email protected]:26257/defaultdb> CREATE TABLE t (a INT PRIMARY KEY);
CREATE TABLE


Time: 16ms total (execution 15ms / network 0ms)

[email protected]:26257/defaultdb> ALTER TABLE t ADD COLUMN b INT PRIMARY KEY;
ERROR: internal error: error executing PreCommitPhase stage 1 of 1 with 9 MutationType ops: relation "t" (104): duplicate constraint name: "crdb_internal_index_3_name_placeholder"
SQLSTATE: XX000
DETAIL: • Schema change plan for ALTER TABLE ‹defaultdb›.public.‹t› ADD COLUMN ‹b› INT8 PRIMARY KEY;
│
├── • PreCommitPhase

jasonmchan pushed a commit to jasonmchan/cockroach that referenced this issue Jun 9, 2022
Previously, the behavior of ALTER TABLE ... ADD COLUMN ... PRIMARY KEY
was undefined. With the legacy schema changer, this statement would
appear to succeed, but it would break the new schema changer. This
commit changes this by returning an explicit error.

In the future, we should support this statement if the table's primary
key was originally the default rowid primary key (see cockroachdb#81449).

Release note: None
jasonmchan pushed a commit to jasonmchan/cockroach that referenced this issue Jun 10, 2022
Previously, the behavior of ALTER TABLE ... ADD COLUMN ... PRIMARY KEY
was undefined. With the legacy schema changer, this statement would
appear to succeed, but it would break the new schema changer. This
commit changes this by returning an explicit error.

In the future, we should support this statement if the table's primary
key was originally the default rowid primary key (see cockroachdb#82735).

Fixes cockroachdb#81449

Release note: None
@craig craig bot closed this as completed in 149137b Jun 14, 2022
@exalate-issue-sync exalate-issue-sync bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) and removed T-sql-schema-deprecated Use T-sql-foundations instead labels May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-schema-changes C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Development

No branches or pull requests

4 participants