-
Notifications
You must be signed in to change notification settings - Fork 172
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
Avoid sending Z
packet in the middle of extended protocol packet sequence if we fail to get connection form pool
#137
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Z
packet in the middle of extended protocol packet sequence if we fail to get connection form pool
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes it possible to run this test script on any architecture
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 is a very good find, thank you.
Great debugging @drdrsh |
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
Q
packet).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
SystemError
and aReadyForQuery
packets. 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
ReadyForQuery
packets 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 onlyS
goes 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 whichS
wants 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.