pgwire: de-strictify extended protocol handling #51194
Merged
+24
−19
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
yuzefovich
approved these changes
Jul 9, 2020
There was a problem hiding this comment.
LGTM, but definitely someone else should sanity check the change as well.
nit (in release note): s/while extended/while in extended/.
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @mjibson, and @rafiss)
maddyblue
approved these changes
Jul 9, 2020
andreimatei
approved these changes
Jul 9, 2020
There was a problem hiding this comment.
LGTM
Maybe find a place in conn.go to put a TODO about the postgres error behavior
Reviewable status:
Previously, the protocol handler didn't permit simple queries during the extended protocol mode. I think this was done because the spec vaguely says that extended protocol commands must be ended with a SYNC command: https://www.postgresql.org/docs/9.3/protocol-flow.html#PROTOCOL-FLOW-EXT-QUERY However, an examination of the PostgreSQL source code shows that the extended protocol mode bit is only used for error recovery. If an error is encountered during extended protocol mode in Postgres, all commands are skipped until the next Sync. CockroachDB never implemented that behavior - instead, it used the extended protocol mode bit only to reject simple queries if they came before the Sync. This commit removes the extended protocol mode bit, as the use case we used it for was incorrect. It's unclear to me whether we need to re-add the bit for dealing with error cases, but we haven't needed it yet. Adding that might be follow-on work, and won't come in this PR. Release note (bug fix): prevent spurious "SimpleQuery not allowed while in extended protocol mode" errors.
jordanlewis
force-pushed
the
fix-protocol-bug
branch
from
275ec2e to
a659313
Compare
|
TFTRs! Thoughts on a backport? I'm in favor, and I'll open one, but curious if there's dissent. bors r+ |
|
+1 for backport |
|
I support backporting |
Build succeeded |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #33693.
Fixes #41511.
Previously, the protocol handler didn't permit simple queries during the
extended protocol mode. I think this was done because the spec vaguely
says that extended protocol commands must be ended with a SYNC command:
https://www.postgresql.org/docs/9.3/protocol-flow.html#PROTOCOL-FLOW-EXT-QUERY
However, an examination of the PostgreSQL source code shows that the
extended protocol mode bit is only used for error recovery. If an error
is encountered during extended protocol mode in Postgres, all commands
are skipped until the next Sync.
CockroachDB never implemented that behavior - instead, it used the
extended protocol mode bit only to reject simple queries if they came
before the Sync.
This commit removes the extended protocol mode bit, as the use case we
used it for was incorrect. It's unclear to me whether we need to re-add
the bit for dealing with error cases, but we haven't needed it yet.
Adding that might be follow-on work, and won't come in this PR.
Release note (bug fix): prevent spurious "SimpleQuery not allowed while
in extended protocol mode" errors.