Conversation
One call and one placeholder pass per write instead of per chunk. Stop on an unexpected release instead of staying checked out.
Contributor
|
@v0idpwn okay, this is some eye-opening stuff. Love this!🫡 I'm presuming the perf hit shouldn't be much more than the previous implementation that we calculated in #1079 (comment).. |
Owns the prepared statement cache, undoing changes the backend didn't carry out. Eviction Closes go before the write's first prepared statement packet, and Closes for statements the backend doesn't have are answered in place.
| end) | ||
| end | ||
|
|
||
| defp extended_query, do: Supavisor.Protocol.Server.extended_query("SELECT 1") |
|
|
||
| rfq = <<?Z, 5::32, ?I>> | ||
| from = {self(), make_ref()} | ||
| limit = Supavisor.Protocol.PreparedStatements.backend_limit() |
v0idpwn
marked this pull request as ready for review
September 25, 2026 17:51
BackendConnection.recv/2 walked the backend's bytes one message at a time and cut a slice per message. A DataRow went through the streaming state even when it had been read whole, so each row cost two record updates, a forward?/2 check that peeks the request queue, and its own element in the iodata written to the client. A message that doesn't move the backend along, like DataRow, can't change where the next one goes. A run of them is now skipped over in one loop and forwarded or dropped with a single forward?/2 check. Streaming is only used for a message that continues past the read. What goes to the client is cut out of the read as ranges, one per run of consecutive forwarded messages. When every message is forwarded, the client gets the read binary itself. recv/2 also no longer appends the read to an empty buffer, which copied it. On a 1,000-row response, recv/2 goes from ~140 µs to ~2.7 µs.
Removes most per-message local calls in recv/2, and lets the compiler update the record in place on ReadyForQuery instead of copying it twice. 100 x recv/2, before -> after: extended, 1 row: 16.95K -> 11.28K reductions, 171.9KB -> 163.3KB extended, 100 rows: 26.85K -> 21.18K reductions, 171.9KB -> 163.3KB simple, 1 row: 10.18K -> 6.68K reductions, 93.0KB -> 84.4KB
Co-authored-by: felipe stival <v0idpwn@gmail.com>
v0idpwn
force-pushed
the
fix/transaction-pipelining-edge-cases
branch
from
September 26, 2026 23:40
99a350c to
134b70f
Compare
This branch has not been deployed
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
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.
In transaction mode, the DbHandler decided a batch was done by counting
ReadyForQuery messages: the ClientHandler told it how many to expect, one
per Query, Sync or FunctionCall in each write. #1205 then added an
open_batch? flag for extended batches still waiting on a Sync.
That model breaks in a few cases:
After an error in an extended protocol message, the backend discards
everything up to the next Sync, so Queries later in the same pipeline
never produce the ReadyForQuery we counted on.
During an extended protocol COPY FROM STDIN, the backend ignores Syncs.
libpq sends them anyway, so we waited for ReadyForQuery messages that
never came.
The Parse and Close messages we inject for prepared statements weren't
always placed correctly when pipelining, and a Parse the backend ignored
after an error stayed in our cache although the statement was never
created.
The DbHandler checked itself back into the pool as soon as it saw the
final ReadyForQuery, so a write the ClientHandler had already forwarded
could land on a connection that was back in the pool.
Separately, the ClientHandler answered any packet starting with a Sync
while idle, including a Sync pipelined together with a query.
This PR follows the backend message by message instead. The ClientHandler
records the tag of each tracked message it forwards, and announces them to
the DbHandler with expect_messages/3 before writing them, replacing
expect_ready_for_query/3. Prepared statement packets are announced as
placeholders, since the DbHandler decides what is actually sent for them.
BackendMessageHandler is replaced by BackendConnection, a pure model of
the backend connection. It queues the messages the backend still has to
answer and consumes them as the backend does, including the
ignore-till-sync and copy-in states. It also owns the prepared statement
cache now: Parses and Closes the backend fails or ignores are undone,
eviction Closes are sent before the write's first prepared statement
packet rather than after the write, and Closes for statements the backend
doesn't have are answered in place. DISCARD ALL and set_application_name
go through the same model, instead of buffering until the data ends in a
ReadyForQuery.
Release is now driven by the ClientHandler. When the backend catches up,
the DbHandler reports the last write it has seen. The ClientHandler
ignores this if a later write still has messages to answer, and otherwise
releases the DbHandler with its own latest write. The DbHandler checks in
if the two match, and otherwise logs an error and stops.
The new tests in transaction_pipelining_test.exs cover pipelined batches
(leading and bare Syncs, several Executes under one Sync, mixed simple and
extended messages, one message per write, writes split into small chunks),
errors (not affecting the next batch, holding the backend until the Sync,
including across a Flush, and a Query discarded after an extended error),
transactions spanning writes in both protocols, including a failed one
held until ROLLBACK, COPY FROM STDIN in both protocols with libpq's Syncs,
CopyFail, a COPY that fails to start or on bad data, COPY TO STDOUT, named
prepared statements (close and re-parse in one write, reuse across writes
and backends, a Parse the backend already has, a Parse sent again after
the backend ignored it), and races between a write and the previous
reply.