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: allow parsing of CREATE [TEMP] TABLE ... ON COMMIT #46594

Merged
merged 1 commit into from Mar 26, 2020

Conversation

otan
Copy link
Contributor

@otan otan commented Mar 25, 2020

Resolves #46555.

PostgreSQL allows temp tables to have custom behavior ON COMMIT to drop
the table or delete the rows that have been added. We do not support
this behaviour, but we do for the default (PRESERVE ROWS).

Add parsing for ON COMMIT, erroring for the DELETE ROWS and DROP case.
Furthermore, add the same error message as PostgreSQL if a non-temporary
table has the ON COMMIT keyword.

Release justification: low risk, high benefit changes to existing
functionality

Release note (sql change): Allow the ON COMMIT syntax for CREATE TABLE.

@otan otan requested review from rohany and a team March 25, 2020 22:40
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

I'll let Rohan comment on correctnes, but I'd like to challenge y'all to start thinking about this question:

how can we teach current and future SQL team members to systematically return error objects with linked issues for unimplemented / partially implemented features?

It's not sustainable for me to catch this all the time. We need a better way.

docs/generated/sql/bnf/create_table_as_stmt.bnf Outdated Show resolved Hide resolved
pkg/sql/parser/sql.y Show resolved Hide resolved
pkg/sql/create_table.go Outdated Show resolved Hide resolved
@otan
Copy link
Contributor Author

otan commented Mar 25, 2020

how can we teach current and future SQL team members to systematically return error objects with linked issues for unimplemented / partially implemented features?

ML/AI 🤓

on a serious note, i think this is a just a thing for knowledge sharing and reinforcement. i'm trying to think "can we add a linter for it", and the answer is "probably for sql.y" but not inside the go files.

pkg/sql/create_table.go Outdated Show resolved Hide resolved
@rohany
Copy link
Contributor

rohany commented Mar 26, 2020

LGTM aside from missing help

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

LGTM ditto rohan

Reviewed 2 of 10 files at r1, 2 of 7 files at r2, 1 of 1 files at r3.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @otan)

PostgreSQL allows temp tables to have custom behavior ON COMMIT to drop
the table or delete the rows that have been added. We do not support
this behaviour, but we do for the default (PRESERVE ROWS).

Add parsing for ON COMMIT, erroring for the DELETE ROWS and DROP case.
Furthermore, add the same error message as PostgreSQL if a non-temporary
table has the ON COMMIT keyword.

Release justification: low risk, high benefit changes to existing
functionality

Release note (sql change): Allow the ON COMMIT syntax for CREATE TABLE.
@otan
Copy link
Contributor Author

otan commented Mar 26, 2020

bors r=rohany,knz

@craig
Copy link
Contributor

craig bot commented Mar 26, 2020

Build succeeded

@craig craig bot merged commit d445101 into cockroachdb:master Mar 26, 2020
SQL Features (Deprecated - use SQL Experience board) automation moved this from Open PRs to Done Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

sql: on commit rows needs an unimplemented error
4 participants