Avoid sending Z packet in the middle of extended protocol packet sequence if we fail to get connection form pool
#137
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
levkk
reviewed
Aug 23, 2022
tests/ruby/tests.rb
Outdated
| conn_str = "postgres://sharding_user:sharding_user@127.0.0.1:6432/sharded_db" | ||
| conn_under_test = PG::connect(conn_str) | ||
| 50.times do | ||
| Thread.new do |
There was a problem hiding this comment.
Curious why you need threads here, I thought async_exec is asynchronous so you could issue multiple queries without waiting for the result? Maybe it's not named correctly.
There was a problem hiding this comment.
My understanding is that async_exec blocks the caller but releases the GVL whereas sync_exec blocks the client and does not release the GVL. I haven't studied the code closely so I could be wrong.
|
Okay, I have a test failing without my fix.
|
drdrsh
changed the title
Z packet in the middle of extended protocol packet sequence if we fail to get connection form pool
drdrsh
commented
Aug 23, 2022
Comment on lines
+22
to
+23
| wget -O toxiproxy-2.4.0.deb https://github.com/Shopify/toxiproxy/releases/download/v2.4.0/toxiproxy_2.4.0_linux_$(dpkg --print-architecture).deb | ||
| sudo dpkg -i toxiproxy-2.4.0.deb |
There was a problem hiding this comment.
Makes it possible to run this test script on any architecture
levkk
approved these changes
Aug 23, 2022
Closed
|
Great debugging @drdrsh |
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.
Sending a query in the extended protocol is done by transmitting a sequence of discrete protocol messages (in contrast with simple mode where the entire query is sent in a
Qpacket).This poses a problem to Pgcat if the pool is busy because if we are unable to checkout a connection from the pool we send back to the client a
SystemErrorand aReadyForQuerypackets. This is unexpected by client (Getting ReadyForQuery before sendingS). This resulted in error messages likeIn this fix, we will hold off on sending back the error and the
ReadyForQuerypackets if we fail to get a connection from the pool while we are processing extended protocol messages other thanS. This will expose us to an edge case if all extended protocol messages end up being discarded because we have no available connections and onlySgoes through in which case the query will fail (which is appropriate) with the error messageunnamed prepared statement does not exist, because Pgcat has discarded the packets that were supposed to build the prepared statement whichSwants to run.Pgbouncer doesn't handle this case simply because failure to obtain a connection from the pool is a fatal error in Pgbouncer that terminates the client connection.