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
tree: make SET TRANSACTION a walkable statement #113689
Conversation
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
0ed6fb9
to
992ed99
Compare
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.
Change LGTM but I don't really see how this fixes the bug. Is there an issue to reference or maybe a quick example that could be added to the commit message?
I think the test I added is the example I had in mind. it allows the SET TRANSACTION statement to be prepared with a placeholder, but before this change, that test would fail. what else would you like to see? |
SET TRANSACTION is now a walkable statement. This fixes a bug where placeholders in a SET TRANSACTION statement were not resolved correctly. The statement must be walkable for the placeholderAnnotationVisitor to work correctly -- it is an AST visitor that relies on walkStmt to discover all placeholders in a statement. Release note (bug fix): Placeholder arguments can now be used in SET TRANSACTION statements.
992ed99
to
1ea4f20
Compare
I have added a code reference to the commit message. |
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.
tftr! bors r+ |
Build failed: |
bors r+ |
Build succeeded: |
SET TRANSACTION is now a walkable statement. This fixes a bug where
placeholders in a SET TRANSACTION statement were not resolved correctly.
The statement must be walkable for the placeholderAnnotationVisitor to
work correctly -- it is an AST visitor that relies on walkStmt to
discover all placeholders in a statement.
cockroach/pkg/sql/sem/tree/type_check.go
Lines 3324 to 3341 in 536ff94
Epic: None
Release note (bug fix): Placeholder arguments can now be used in SET TRANSACTION statements.