Fix COPY FROM STDIN handling #1025
Merged
Conversation
This file contains hidden or 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
JelteF
force-pushed
the
fix-copy-in-prepared-statement
branch
2 times, most recently
from
8b0532e to
e63a15c
Compare
JelteF
force-pushed
the
fix-copy-in-prepared-statement
branch
from
e63a15c to
2e180f8
Compare
JelteF
force-pushed
the
fix-copy-in-prepared-statement
branch
from
2e180f8 to
9aeeac4
Compare
JelteF
commented
Feb 21, 2024
src/server.c
Outdated
| if (!clear_outstanding_requests_until(server, "cf")) | ||
| return false; | ||
| } else { | ||
| server->query_failed = true; |
Member
Author
There was a problem hiding this comment.
This should happen always probably, otherwise we won't pop a later received CopyDone message in case of failure.
In pgbouncer#854 full tracking of expected responses was implemented. This turned out this have bugs when sending `COPY FROM STDIN` queries using the extended protocol. The protocol explicitely allows sending `Sync` messages after sending an `Execute` message for a `COPY FROM STDIN` command. Such Sync messages are ignored by the server until a CopyDone or CopyFail message is received. The new command queue tracking did not take this into account, and was incorrectly expecting more ReadyForQuery messages from the server. This PR fixes that issue. It also removes the rfq_count tracking, since it's now only keeping track of a strict subset of what the outstanding request queue was tracking. Fixes pgbouncer#1023 Fixes pgbouncer#1024 TODO: - [ ] Add tests for COPY TO STDOUT
JelteF
force-pushed
the
fix-copy-in-prepared-statement
branch
from
e3048a3 to
0c7ca30
Compare
JelteF
force-pushed
the
fix-copy-in-prepared-statement
branch
from
0c7ca30 to
3c25bae
Compare
Member
|
There are some tests that still refers to table "t". |
Member
Author
ugh, fixed |
JelteF
added a commit
to JelteF/pgbouncer
that referenced
this pull request
Mar 6, 2024
I realized I was a bit too trigger-happy when removing all references to expected_rfq_count in pgbouncer#1025. One occurrence should have been replaced with adding an outstanding request.
JelteF
added a commit
to JelteF/pgbouncer
that referenced
this pull request
Mar 6, 2024
I realized I was a bit too trigger-happy when removing all references to expected_rfq_count in pgbouncer#1025. One occurrence should have been replaced with adding an outstanding request.
JelteF
added a commit
that referenced
this pull request
Apr 10, 2024
I realized I was a bit too trigger-happy when removing all references to `expected_rfq_count` in #1025. One occurrence should have been replaced with adding an outstanding request. Normally this isn't an issue, but it could cause the server connection not to be closed if the client disconnects before the auth_query is finished running. To be clear, #1025 did not introduce a regression in this respect. Because `expect_rfq_count` being non-zero on would not cause a server connection closure when the server was released.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
In #845 full tracking of expected responses was implemented. This turned out to have bugs when sending
COPY FROM STDINqueries using the extended protocol. The protocol explicitly allows sendingSyncmessages after sending anExecutemessage for aCOPY FROM STDINcommand. Such Sync messages are ignored by the server until a CopyDone or CopyFail message is received. The new command queue tracking did not take this into account, and was incorrectly expecting more ReadyForQuery messages from the server.This could lead to two problems for the server connection on which the
COPY FROM STDINwas sent:max_prepared_statementswas non-zero, then clients might receive ParseComplete and CloseComplete responses in the wrong order. This wouldn't result in wrong behaviour by the server, but the client might get confused and close the connection.This PR fixes these issues. It also removes the rfq_count tracking, since it's now only keeping track of a strict subset of what the outstanding request queue was tracking.
Fixes #1023
Fixes #1024