On Wed, Sep 9, 2026 at 6:04 PM Dave Cramer <[email protected]> wrote:
> Attached is v5, rebased onto master and split into three patches
Hi Dave,
Before more work goes into the details, I'd like to discuss the wire
format. Once _pq_.cursor is negotiated, the format of every Bind and
every Execute on the connection changes, and that is hard on connection
poolers. I think holdability can be sent in a message of its own, which
would leave Bind and Execute unchanged and make 0002 optional. Many of
the remaining problems depend on which format is chosen, so I've listed
them last.
The PgBouncer results below come from v5 without 0004; 0004 does not
change the behavior they show. The rest was tested on v5 with 0004 and
0005 applied (my comments on those are in reply to Sehrope).
1. What a pooler sees
The changed format also applies to Bind and Execute messages sent by
code that never uses a cursor option. For a pooler, that has two
consequences:
- It cannot share a server connection between clients that negotiated
the option and clients that did not; it needs separate pools or has
to rewrite messages.
- If a middlebox drops _pq_.cursor without sending
NegotiateProtocolVersion, libpq assumes the option was accepted, and
every extended query fails. With PgBouncer 1.22.1 and
ignore_startup_parameters = _pq_.cursor, psql's
"select $1 \bind 42 \g" fails with "invalid message format".
(PgBouncer 1.23 and later send NegotiateProtocolVersion themselves and
decline the option. They are safe today, but the feature is not
available through them.)
2. Which statements need HOLD in the protocol
The strongest case for holding portals in the protocol is the result of
INSERT, UPDATE, DELETE, or MERGE ... RETURNING, or of a data-modifying
WITH, because SQL cannot keep it open across COMMIT at all. With 0004,
HOLD is limited to a single SELECT, and for a SELECT drivers can already
get a holdable cursor from DECLARE ... WITH HOLD. Portals for RETURNING,
for a data-modifying WITH, and for row-returning utility statements
already run to completion into a tuplestore, so holding them after
their first Execute looks tractable. What COMMIT should do with such a
portal that was never executed is an open question. I'd see holding
these portals as a follow-up rather than something for this patch.
3. HOLD in a message of its own
CURSOR_OPT_HOLD is read only by PreCommit_Portals and pg_cursors, not by
PortalStart or the planner, so holdability does not have to arrive with
Bind. A separate message would leave Bind and Execute unchanged. For
example:
PortalHold (F) String portal name
PortalHoldComplete (B)
The client would send PortalHold after the Bind of a named portal and
before the transaction ends. The server would accept it only if the
option was negotiated.
The separate message changes three things for a pooler. It can enable
the option on every server connection. A dropped startup parameter
breaks only the new message. And the request to hold is a message of its
own, rather than the last four bytes of a Bind whose tail PgBouncer
streams without buffering. It would also leave libpq with a single Bind
encoder. With v5 applied, libpq has two, and the two PQsendBindGuts
bugs in section 5 are in the second one.
Supporting HOLD itself in transaction pooling costs the same with either
format: the pooler has to keep the server connection for the client
while a held portal exists, because the portal and its name live on that
connection. Section 4 asks how a pooler would know when that ends.
SCROLL has to be known at PortalStart, so if SCROLL stays in the patch,
a variant of the Bind message would be the natural place for it, again
without changing Bind itself. SQL FETCH and MOVE can already read, by
name and in any direction, a portal that 0004 makes scrollable. Are the
Execute fields in 0002 needed in this patch at all?
4. A question on session state
A held portal outlives its transaction. I ran a PgBouncer build patched
to forward the option in transaction pooling mode, and three things
happened. The client that created the portal lost it at the client's
next transaction. Another client could fetch from the portal. And the
portal collided with the next client's ordinary portal of the same name
(42P03, cursor "C_1" already exists; pgjdbc names portals C_1, C_2, ...
on every connection).
To keep the server connection for that client while the portal exists,
a pooler has to detect when the portal is dropped, and it cannot detect
every way that happens: Close, SQL CLOSE, DISCARD ALL, or a failed
transaction. Hannu mentioned a ReadyForQuery flag for held cursors
(https://postgr.es/m/CAMT0RQRFa43CAf773LaDUvBYmZtwcNfdQSAzpBUtdJ+EiHH=k...@mail.gmail.com).
I'm not sure a generic "the session has state" signal would be useful
instead, because changed GUCs, prepared statements, temp tables,
advisory locks, and LISTEN are session state too, and poolers already
track some of them. Is a portal-specific signal worth adding? Or is it
enough for now to say in protocol.sgml that poolers in transaction
pooling mode cannot support HOLD?
5. Other problems in v5 that 0004 does not fix
The problems below depend on the format. None of them remains if Bind
and Execute stay unchanged or 0002 is dropped:
- A directional Execute runs a PORTAL_UTIL_SELECT portal with
isTopLevel = false. For a procedure that has an INOUT parameter and
commits, "call p(null)" returns 42 through a plain Execute and fails
with 2D000 "invalid transaction termination" through an Execute with
FORWARD ALL.
- The server validates the direction field after the portal lookup, and
after log_statement has logged the Execute: an Execute with direction
5 on a missing portal fails with 34000 rather than 08P01.
- protocol.sgml says the server never sends PortalSuspended in response
to an Execute under the extension. With all-zero fetch fields and a
nonzero row limit the server still sends it, as the 0002 commit
message says. The code is right and protocol.sgml is wrong.
- 0002 without 0003 breaks every PQexecParams on a connection with
protocol_cursor=1 ("insufficient data left in message"), so the series
cannot be bisected at 0002.
- A MOVE through PQsendExecutePortal returns PGRES_TUPLES_OK after a
Describe, where SQL MOVE returns PGRES_COMMAND_OK.
- libpq's PQsendBindGuts has two bugs:
* with paramLengths NULL it calls strlen() on a binary parameter;
AddressSanitizer reports a heap-buffer-overflow (READ of size 5 on a
4-byte value), where PQsendQueryPrepared refuses the call with
"length must be given for binary parameter".
* it uses paramLengths for text parameters. For
"select $1::text || '|' || length($1::text)" with "3" and length 0,
PQsendBindAndExecutePortal returns "|0", while PQexecPrepared with
the same arrays returns "3|1".
The problems below do not depend on the format:
- libpq accepts a NegotiateProtocolVersion that names _pq_.cursor even
when the client did not request it, while any other unrequested name
fails the connection.
- Outside a transaction block, PQsendBindWithCursorOptions without HOLD
returns PGRES_COMMAND_OK, but the server has already dropped the
portal at the Sync that call sent, so the next PQsendExecutePortal
gets 34000.
- PQprotocolCursorEnabled returns 1 for a connection still waiting for
the startup reply, and for a connection that failed after startup
(database does not exist).
- libpq reads only the first character of protocol_cursor, so "true",
"on", and "yes" silently leave the extension off.
- conn->protocol_cursor is never freed (2 bytes per PGconn and per
PQcancelCreate).
Regards,
Vladimir
