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: capture sequences used in cast exprs #62203

Merged
merged 1 commit into from
Mar 19, 2021
Merged

sql: capture sequences used in cast exprs #62203

merged 1 commit into from
Mar 19, 2021

Conversation

the-ericwang35
Copy link
Contributor

@the-ericwang35 the-ericwang35 commented Mar 18, 2021

Previously, GetSequenceFromFunc only looked
for *tree.DString and *tree.DOid. However,
it's also possible for the sequence to be given via
a *tree.CastExpr (for example, nextval('s'::regclass)).
In this case, the sequence was not being captured,
which meant no back references were created and
no sequence encoding was done.
This PR addresses this by adding an additional
case to capture these sequences.

This will probably need to be backported.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@the-ericwang35 the-ericwang35 requested a review from ajwerner March 18, 2021 16:31
Previously, `GetSequenceFromFunc` only looked
for `*tree.DString` and `*tree.DOid`. However,
it's also possible for the sequence to be given via
a CastExpr (for example, nextval('s'::regclass)).
In this case, the sequence was not being captured,
which meant no back references were created and
no sequence encoding was done.
This PR addresses this by adding an additional
case to capture sequences used in CastExpr.

Release note: None
Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

This needs a backport. I worry that there are some other crazy cases where we might wish we had some constant folding that I care a bit less about.

Nice catch. What helped you find this? Is it that in the later PR we actually do serialize them like this?

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

@the-ericwang35
Copy link
Contributor Author

Thanks for the review!

Nice catch. What helped you find this? Is it that in the later PR we actually do serialize them like this?

It was because while writing the PR for sequences in views, I noticed we didn't allow regclass IDs in views (doing something like CREATE VIEW v AS (SELECT nextval(59::regclass)) results in ERROR: relation "59" does not exist). This code was the reason for this, since it always assumes the value being cast to regclass is a table name (I've since changed this to allow for IDs in the views/sequences PR). This made me realize I never tested this table name regclass case, and lo and behold it didn't work.

@the-ericwang35
Copy link
Contributor Author

bors r=ajwerner

@craig
Copy link
Contributor

craig bot commented Mar 19, 2021

Build succeeded:

@craig craig bot merged commit 67c7c7b into cockroachdb:master Mar 19, 2021
@the-ericwang35 the-ericwang35 deleted the ericwang/cast_sequence branch March 19, 2021 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants