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.

Sign up for GitHub

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

Jump to bottom

sql: drop rowid column when ADD PRIMARY KEY overwrites the hidden rowid column #80149

Closed
otan opened this issue Apr 19, 2022 · 0 comments · Fixed by #86071
Closed

sql: drop rowid column when ADD PRIMARY KEY overwrites the hidden rowid column #80149

otan opened this issue Apr 19, 2022 · 0 comments · Fixed by #86071
Labels
A-tools-aws-dms Blocking support for AWS Database Migration Service C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@otan
Copy link
Contributor

otan commented Apr 19, 2022 •
edited by cockroach-jira-scripts
Loading

Today, ADD PRIMARY KEY will not drop the rowid column if it was the default.

We should DROP this column if:

  • ADD PRIMARY KEY is used
  • rowid is the non-visible hidden column

There was an attempt to do this in #78476, but we has bugs. We anticipate doing this in the new schema changer.

Slack discussion

Jira issue: CRDB-15832
@otan otan added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Apr 19, 2022
@blathers-crl blathers-crl bot added the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label Apr 19, 2022
@blathers-crl blathers-crl bot added the T-sql-schema-deprecated Use T-sql-foundations instead label Apr 21, 2022
@otan otan added the A-tools-aws-dms Blocking support for AWS Database Migration Service label Apr 26, 2022
@jlinder jlinder added sync-me and removed sync-me labels May 20, 2022
This was referenced Aug 11, 2022
Closed
Closed
postamar pushed a commit to postamar/cockroach that referenced this issue Aug 12, 2022
scbuild: primary index swap and drop column improvements
3c70cc2 
This commit rewrites parts of the declarative schema changer builder to
pave the way for dropping the rowid column when altering primary keys.
These changes should have very little impact on functionality.

Informs cockroachdb#80149.

Release note: None
postamar pushed a commit to postamar/cockroach that referenced this issue Aug 12, 2022
scbuild: primary index swap and drop column improvements
15ba065 
This commit rewrites parts of the declarative schema changer builder to
pave the way for dropping the rowid column when altering primary keys.
These changes should have very little impact on functionality.

Informs cockroachdb#80149.

Release note: None
postamar pushed a commit to postamar/cockroach that referenced this issue Aug 12, 2022
opgen,rules: first-class support for transient targets
a37c276 
This commit makes it possible to easily declare a transient target path
in opgen for any element, based on its public and absent paths. The
transient path is generated by taking these two and stitching them
together, so to speak.

This is motivated by the need to have a transient path for the primary
index element, to in turn support ALTER TABLE ... ADD COLUMN, DROP COLUMN
statements and especially in the near term, an ALTER PRIMARY KEY
implementation which drops the rowid column.

This change in turn forced a rewriting of the rules, so this was deemed
as good an opportunity as ever to streamline the index and column
dependency rules a bit.

Informs cockroachdb#80149.

Release note: None
postamar pushed a commit to postamar/cockroach that referenced this issue Aug 12, 2022
scbuildstmt: ALTER PK drops rowid column when possible
24d08e6 
This commit improves the ALTER TABLE ... ALTER PRIMARY KEY ...
implementation in the declarative schema changer by also having it drop
the hidden rowid column when possible.

This required introducing a transient public index which includes rowid
as a storing column. This transient index is then swapped out with
the definitive primary index which does not include rowid at all. This
commit adds a rule to ensure that the second swapping takes place only
after the original primary index, in which rowid was the PK, has been
completely removed. Otherwise, we run into some nasty issues when faced
with concurrent writes.

Fixes cockroachdb#80149.

Release note (sql change): When performed by the declarative schema
changer (as is the case by default) the ALTER PRIMARY KEY statement
now also drops the rowid column when no references are held to it
anywhere. The rowid column is a hidden column which is implicitly added
and serves as primary key on any table which is created without
explicitly specifying a primary key.
@postamar postamar mentioned this issue Aug 12, 2022
Merged
postamar pushed a commit to postamar/cockroach that referenced this issue Aug 13, 2022
scbuild: primary index swap and drop column improvements
26c48ff 
This commit rewrites parts of the declarative schema changer builder to
pave the way for dropping the rowid column when altering primary keys.
These changes should have very little impact on functionality.

Informs cockroachdb#80149.

Release note: None
postamar pushed a commit to postamar/cockroach that referenced this issue Aug 13, 2022
opgen,rules: first-class support for transient targets
6993039 
This commit makes it possible to easily declare a transient target path
in opgen for any element, based on its public and absent paths. The
transient path is generated by taking these two and stitching them
together, so to speak.

This is motivated by the need to have a transient path for the primary
index element, to in turn support ALTER TABLE ... ADD COLUMN, DROP COLUMN
statements and especially in the near term, an ALTER PRIMARY KEY
implementation which drops the rowid column.

This change in turn forced a rewriting of the rules, so this was deemed
as good an opportunity as ever to streamline the index and column
dependency rules a bit.

Informs cockroachdb#80149.

Release note: None
craig bot pushed a commit that referenced this issue Aug 13, 2022
@ajwerner
Merge #86071
aa77fba 
86071: scbuildstmt: ALTER PK drops rowid column when possible r=postamar a=postamar

This commit improves the ALTER TABLE ... ALTER PRIMARY KEY ...
implementation in the declarative schema changer by also having it drop
the hidden rowid column when possible.

This required introducing a transient public index which includes rowid
as a storing column. This transient index is then swapped out with
the definitive primary index which does not include rowid at all. This
commit adds a rule to ensure that the second swapping takes place only
after the original primary index, in which rowid was the PK, has been
completely removed. Otherwise, we run into some nasty issues when faced
with concurrent writes.

Fixes #80149.

Release note (sql change): When performed by the declarative schema
changer (as is the case by default) the ALTER PRIMARY KEY statement
now also drops the rowid column when no references are held to it
anywhere. The rowid column is a hidden column which is implicitly added
and serves as primary key on any table which is created without
explicitly specifying a primary key.


Co-authored-by: Marius Posta <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
@craig craig bot closed this as completed in c15d1ad Aug 13, 2022
@healthy-pod healthy-pod removed the T-sql-schema-deprecated Use T-sql-foundations instead label May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-tools-aws-dms Blocking support for AWS Database Migration Service C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Milestone
No milestone
3 participants
@jlinder @otan @healthy-pod