fix: buffer Close messages in extended query protocol - #4951
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the handling of PostgreSQL Close messages by immediately unregistering statements and portals from the connection map during buffering, while deferring their actual closure until the flush phase. This allows correct state unwinding in reverse (LIFO) order if a pipeline fails, which is thoroughly verified by a new suite of mock server tests. The review feedback correctly identifies a performance improvement in ExtendedQueryProtocolHandler where changing the messages list from LinkedList to ArrayList avoids O(N^2) complexity during the reverse abort loop.
1343e40 to
a87f960
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the handling of PostgreSQL Close messages in PGAdapter by deferring resource cleanup until flush time, allowing correct LIFO unwinding of state mutations in case of pipeline failures. It replaces the LinkedList of messages in ExtendedQueryProtocolHandler with an ArrayList that dynamically trims its capacity, and adds comprehensive mock server tests. The review feedback suggests catching and logging exceptions during the message abort loop in ExtendedQueryProtocolHandler to ensure that a failure in aborting one message does not block the abort process for subsequent messages in the pipeline.
Buffer Close ('C') messages in ExtendedQueryProtocolHandler until a
Flush or Sync message is received, matching the behavior of other
extended query protocol messages.
Previously, Close messages were executed eagerly upon receipt. In
pipelined batches (such as Parse + Close + Sync or Execute + Close + Sync),
this caused CloseComplete ('3') to be returned to the client before
responses to earlier messages in the batch. This broke connection poolers
(such as PgBouncer in transaction pooling mode) and client drivers that
track in-flight requests in FIFO order.
Statements and portals are unregistered from ConnectionHandler when
buffering so that subsequent messages in the same pipeline can reuse the
name. Statement closure and CloseComplete response emission are deferred
to flush. If an earlier message in the pipeline fails, buffered messages
are aborted in reverse (LIFO) order, restoring any statement or portal
whose Close was skipped.
a87f960 to
18130b7
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the handling of PostgreSQL Close messages in PGAdapter to support deferred execution and LIFO (Last-In-First-Out) aborts within pipelined queries. Specifically, CloseMessage now extends AbstractQueryProtocolMessage and unregisters portals or statements immediately from the connection map during the buffering phase, while deferring actual resource cleanup until flush. If a pipeline error occurs, remaining messages are aborted in reverse order to correctly unwind state mutations. Additionally, the message buffer in ExtendedQueryProtocolHandler was migrated from a LinkedList to an ArrayList with automatic capacity trimming to optimize memory usage. Comprehensive mock server and unit tests have been added to verify these pipeline behaviors, error recovery, and buffer management. I have no additional feedback to provide.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the handling of PostgreSQL Close messages by deferring actual resource cleanup to the flush phase, while immediately unregistering them from the connection map during buffering. This allows subsequent pipeline messages to reuse names or fail correctly if they attempt to use closed resources. Additionally, it introduces LIFO-ordered aborting of buffered messages on pipeline errors to correctly unwind state mutations, manages buffer capacity dynamically to prevent memory bloat, and adds comprehensive mock server tests to verify various pipeline, error, and transaction scenarios. I have no feedback to provide as there are no review comments.
Buffer Close ('C') messages in ExtendedQueryProtocolHandler until a Flush or Sync message is received, matching the behavior of other extended query protocol messages.
Previously, Close messages were executed eagerly upon receipt. In pipelined batches (such as Parse + Close + Sync or Execute + Close + Sync), this caused CloseComplete ('3') to be returned to the client before responses to earlier messages in the batch. This broke connection poolers (such as PgBouncer in transaction pooling mode) and client drivers that track in-flight requests in FIFO order.
Statements and portals are unregistered from ConnectionHandler when buffering so that subsequent messages in the same pipeline can reuse the name. Statement closure and CloseComplete response emission are deferred to flush. If an earlier message in the pipeline fails, buffered messages are aborted in reverse (LIFO) order, restoring any statement or portal whose Close was skipped.