-
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
release-19.2: sql: allow DEALLOCATE ALL with a prepared statement #52992
Conversation
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.
Thanks for fixing! Noting one diff from #52940 below - if that's expected/okay then LGTM!
{"Type":"CommandComplete","CommandTag":"DISCARD"} | ||
{"Type":"ReadyForQuery","TxStatus":"I"} | ||
|
||
# Send a simple query in the middle of extended protocol, which is apparently |
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.
This already existed in master, but is added in this PR -- I'm guessing that's intended/okay?
071c153
to
fecee65
Compare
fecee65
to
744fde5
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.
I would keep the other pgjdbc test case out of the file since it's unrelated. We probably also need to do a small test rewrite:
--- FAIL: TestPGTest/pgjdbc (0.02s)
datadriven.go:53:
testdata/pgtest/pgjdbc:43: ReadyForQuery
expected:
{"Type":"ParameterStatus","Name":"application_name","Value":""}
{"Type":"ParameterStatus","Name":"TimeZone","Value":"UTC"}
{"Type":"CommandComplete","CommandTag":"DISCARD"}
{"Type":"ReadyForQuery","TxStatus":"I"}
found:
{"Type":"CommandComplete","CommandTag":"DISCARD"}
{"Type":"ReadyForQuery","TxStatus":"I"}
I think this is expected (#42376)
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @rafiss, and @yuzefovich)
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.
thanks for catching those! fixed
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @yuzefovich)
PR cockroachdb#48842 added logic to exhaust portals after executing them. This had issues when the portal being executed closes itself, which happens when using DEALLOCATE in a prepared statement. Now we check if the portal still exists before exhausting it. There is no release note as this fixes a bug that only exists in unreleased versions. Release note: None
744fde5
to
076572c
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.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @rafiss and @yuzefovich)
Backport 1/1 commits from #52940.
/cc @cockroachdb/release
PR #48842 added logic to exhaust portals after executing them. This had
issues when the portal being executed closes itself, which happens when
using DEALLOCATE in a prepared statement. Now we check if the portal
still exists before exhausting it.
There is no release note as this fixes a bug that only exists in
unreleased versions.
fixes #52915
fixes #52220
fixes #52880
fixes #52506
Release note: None