-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
distsql: add remaining seq fns to blacklist #28114
Conversation
There were still a couple of sequence functions that weren't properly blacklisted from running in DistSQL. Release note (bug fix): prevent some sequence builtins from incorrectly running in distributed flows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (and 1 stale)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 1 of 0 LGTMs obtained
pkg/sql/sem/builtins/builtins.go, line 1462 at r1 (raw file):
tree.FunctionProperties{ Category: categorySequences, DistsqlBlacklist: true,
I forgot what we discussed... Wasn't currval supposed to work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 1 of 0 LGTMs obtained
pkg/sql/sem/builtins/builtins.go, line 1462 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
I forgot what we discussed... Wasn't currval supposed to work?
Maybe it should, but the current implementation doesn't let it. I could fix that now too, I suppose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 1 of 0 LGTMs obtained
pkg/sql/sem/builtins/builtins.go, line 1462 at r1 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
Maybe it should, but the current implementation doesn't let it. I could fix that now too, I suppose
I'll fix it in a separate PR.
bors r+ |
👎 Rejected by code reviews |
because i'll fix it in another pr
bors r+ |
28114: distsql: add remaining seq fns to blacklist r=jordanlewis a=jordanlewis There were still a couple of sequence functions that weren't properly blacklisted from running in DistSQL. Release note (bug fix): prevent some sequence builtins from incorrectly running in distributed flows. Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 1 of 0 LGTMs obtained
pkg/sql/sem/builtins/builtins.go, line 1462 at r1 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
I'll fix it in a separate PR.
Wait, no, it's not really possible without major surgery. The problem is that resolving a sequence name (which currval accepts) requires a ton of infrastructure which isn't available on remote flows yet.
Build succeeded |
There were still a couple of sequence functions that weren't properly
blacklisted from running in DistSQL.
Release note (bug fix): prevent some sequence builtins from incorrectly
running in distributed flows.