Skip to content

fix: rework protocol handling to better handle pipelining - #1207

Open
v0idpwn wants to merge 25 commits into
mainfrom
fix/transaction-pipelining-edge-cases
Open

v0idpwn wants to merge 25 commits into
mainfrom
fix/transaction-pipelining-edge-cases

Conversation

@v0idpwn

@v0idpwn v0idpwn commented Sep 23, 2026 •

Copy link
Copy Markdown
Member

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.

@Snehil-Shah

Snehil-Shah commented Sep 23, 2026 •

Copy link
Copy Markdown
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)..

Comment thread test/supavisor/db_handler_test.exs Fixed
Comment thread test/supavisor/db_handler_test.exs Fixed
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.
Comment thread test/supavisor/db_handler_test.exs Fixed
Comment thread test/supavisor/db_handler_test.exs Fixed
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
v0idpwn marked this pull request as ready for review September 25, 2026 17:51
@v0idpwn
v0idpwn requested a review from a team as a code owner September 25, 2026 17:51
v0idpwn and others added 5 commits September 25, 2026 14: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>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants