fix(pgsql): keep a client's session state across a poisoned transaction (#6536)

RA
8h ago
+1234-95

Fixes #6536

With pgsql-preserve_client_on_broken_backend_in_tx (default true) a client whose backend goes away inside a transaction is kept and answered 25P02. It then lost its session state without being told: session advisory locks, temp tables and sequences, SQL PREPAREs, WITH HOLD cursors, LISTEN, untracked SETs. Two more paths lost the same state with a healthy backend: an error ProxySQL raises itself inside a transaction (a rejected DISCARD ALL, a query rule error_msg), then ROLLBACK, dropped the live backend. PostgreSQL keeps the session in all of these.

Fix (4cb13e5be)

  • Backend death: handler_minus1_PoisonTransaction() no longer poisons when the connection holds session state (STATUS_PGSQL_CONNECTION_SESSION_STATE, built from the flags that already disable multiplexing). The client is disconnected, as PostgreSQL does. NO_MULTIPLEX is not in the mask: a multiplex=0 rule says nothing about state.
  • Local error and DISCARD ALL in a transaction: a backend that holds session state is not dropped when the poisoned transaction ends. It is sent a ROLLBACK through the existing reset path, in a rollback-only mode (no DISCARD ALL, no conn->reset(), so its flags and variables stay). The rollback runs as soon as the session is idle. A backend without session state is dropped as before.
  • Packets queued behind the ROLLBACK wait for that rollback. A client may pipeline them; read in the same batch they ran inside the old transaction, were reported done, and were then discarded by the rollback (silent loss of an INSERT). get_pkts_from_client() stops reading while a rollback is owed.
  • A failed or timed-out rollback ends the session: there is no client reply to send an error on.
  • pg_try_advisory_lock sets the advisory lock flag like pg_advisory_lock.
  • A COMMIT that ends a poisoned transaction no longer sends the 25P01 "there is no transaction in progress" warning. PostgreSQL 17 sends none there; the warning is for a COMMIT outside any transaction.

Test (d14706419)

pgsql-tx_poisoned_recovery-t (existing file), 35 checks to 253:

  • case F: a backend killed in a transaction disconnects the client when it held state (lock, temp table, PREPARE, WITH HOLD cursor, temp sequence, LISTEN, untracked SET; taken before, inside or in an earlier transaction, or over the extended protocol), and still poisons it when it did not (savepoint, transaction lock, tracked SET, state released with pg_advisory_unlock_all/DISCARD TEMP/CLOSE ALL, multiplex=0 rule);
  • case G: after an error of ProxySQL's own, ROLLBACK or COMMIT (simple or extended) keeps the session lock and rolls the work back; the next statement on another hostgroup; SET values back after the rollback; statements pipelined behind the ROLLBACK over the simple and the extended protocol (6 rounds each, the failure is timing dependent);
  • case B: a poisoned COMMIT carries no notice;
  • case H: an unanswered COMMIT is found without a usable digest (see below);
  • cases F and G run in four variants per backend protocol, client TLS or not and backend TLS or not, and each variant checks from pg_stat_ssl that the backend leg is what was set.

Results, release PROXYSQL31=1 build on a host ProxySQL with a local PostgreSQL 17, libpq and native mode:

  • this change: 253/253 (4 skips: LISTEN on a libpq backend);
  • without the guard: 17 of the new checks fail; without the packet-loop fix: the 16 pipelining checks fail (0 of 6 rounds right);
  • 400 pipelined poison cycles per protocol at 20 in parallel: every lock kept, nothing committed from the old transaction, no connection left idle in a transaction, no assert;
  • a frozen backend during the proxy's rollback ends the session after connect_timeout_server (3 s), and a server set OFFLINE_HARD before the client's ROLLBACK ends it at once, in both modes; nothing leaked, the proxy kept serving.

Related tests, same counts as before the change: pgsql-discard_all_txn_block-t 100/102 (the two failures are marked # todo), pgsql-extq_local_error_in_txn-t, pgsql-local_error_txn_state-t, pgsql-tx_poisoned_extended_query-t, pgsql-reg_test_6084_tx_poisoned_eventslog-t, pgsql-hold_cursor_pinning-t, pgsql-multiplex_status_test-t, pgsql-native_pool_reset-t, pgsql-extq_implicit_prepare_refused-t, pgsql-transaction_variable_state_tracking-t, pgsql-retry_guard_in_txn_on_broken_backend-t. pgsql-txn_local_answers-t (12/30) and pgsql-txn_tracker_follows_backend-t (48/56) fail the same way with and without this change: COMMIT PREPARED and similar are answered locally as a plain COMMIT with no transaction open, and repeated or nested ROLLBACK TO savepoints are mis-tracked. Neither runs this code (no session state).

Not run: the Docker CI groups; a Stable (v3.0, libpq only) build; the PR branch itself. The numbers above are from a local branch that also carries the fix of #6522; the PR branch differs from it only by that fix (lib/PgSQL_Thread.cpp, groups.json, pgsql-reg_test_6521_cancel_idle_session_busy_poll-t.cpp).

Review follow-ups (26433a9cd, 25888830e)

commit_unanswered() read the statement's digest, which is missing when pgsql-query_digests and pgsql-commands_stats are both off (a COMMIT killed in flight then left the client poisoned) and cut for a query longer than pgsql-query_digests_max_query_length (which disconnected the client after any backend failure). PgSQL_ExplicitTxnStateMgr::commit_in_query() now digests the query itself in full in those cases, as the LISTEN check does, and fails closed if it cannot. Case H covers it: the digests-off checks fail on the previous binary, and so does the control that a long query without a COMMIT still poisons. The check of pipelined statements no longer hangs if the proxy closes the connection.

Not changed, with the reason in the threads: Sonar's goto (the handler_again idiom of the whole function), nesting and cognitive complexity in the test file, std::function in test helpers, and the docstring-coverage warning; none is a defect.

Also in this PR (5da2418dd, 65849395d)

pgsql: do not report an unanswered COMMIT as an aborted transaction, with its test. The change above sits next to its guard in handler_minus1_PoisonTransaction() and does not apply without it. When the backend died while a COMMIT was waiting for its reply, the client was answered 25P02, though the COMMIT may have taken effect (a backend ended by pg_terminate_backend while waiting for a synchronous standby). The client is now disconnected, as PostgreSQL does. It has no issue or PR of its own yet.

DISCARD ALL refused in a transaction or a pipeline (2ff7da3c8)

Before this commit a DISCARD ALL inside a transaction, or pipelined behind another statement, was refused by ProxySQL, and the backend was dropped to be safe. A session advisory lock, temp table or prepared statement was lost, and the client was not told.

  • Inside an explicit transaction the statement goes to the backend. PostgreSQL refuses it and aborts the transaction itself, as it does for any error it raises. The session and its locks are kept. The client gets the same SQLSTATE (25001) and message as before. On the libpq path the NoData owed for a Describe folded into the Execute is still sent.
  • Outside a transaction, on the native backend protocol, a pipelined DISCARD ALL is failed on the backend with the existing extq_fail_on_backend() path (the same one a SELECT 1/0 in the middle of a pipeline uses). The backend fails the unit and keeps the session.
  • Measured, both protocols, lock held, pg_locks checked from a second session, against direct PostgreSQL: DISCARD ALL alone in a transaction, extended protocol in a transaction, pipelined in a transaction: kept on native and libpq. Pipelined with no transaction: kept on native; still lost on libpq.
  • Case G of pgsql-tx_poisoned_recovery-t used a DISCARD ALL to provoke ProxySQL's own error. That error no longer exists inside a transaction, so case G now provokes it with the query rule it installs.

Contract note. This changes what DISCARD ALL does on the maintainers' v3.0 code, where the refusal was added unreleased in ddd30da59 and d3c0b28cd: the connection is kept where it was dropped, and the statement now reaches the backend inside a transaction. The client-visible error is unchanged. doc/agents/invariants.md says to treat a change that might touch an invariant as one, so I am flagging it for maintainer review. The PR is based on feature/pgsql-native-extq-batching, and by invariant I-2 a correctness fix must not be confined to v3.1+: the poison code path in this PR is Stable code, but the branch base is not. That base question is open.

Results (PROXYSQL31=1 build, host ProxySQL, local PostgreSQL 17, libpq and native):

  • pgsql-tx_poisoned_recovery-t 253/253 (4 skips: LISTEN on libpq);
  • pgsql-discard_all_txn_block-t 100/102, the two failures are its # todo checks; the 8 checks that a disconnect breaks pass;
  • pgsql-native_extq_batch-t 61/61, pgsql-extq_rule_error_stops_batch-t 25/25, pgsql-extq_local_error_in_txn-t 64/64, pgsql-local_error_txn_state-t 64/64, pgsql-hold_cursor_pinning-t 118/118, pgsql-transaction_variable_state_tracking-t 156/156, others unchanged;
  • pgsql-txn_local_answers-t 12/30 and pgsql-txn_tracker_follows_backend-t 48/56 fail as before, without this change too.

Not fixed here

The session-state flags come from matching the start of the query text, so a client is still poisoned and loses the object for CREATE LOCAL TEMP TABLE, CREATE TEMP VIEW, SELECT ... INTO TEMP, set_config(..., false), a lock taken in a function or DO block, anything after the first statement of a multi-statement query, and DISCARD SEQUENCES (which clears the temp sequence flag but drops nothing). On libpq, a DISCARD ALL pipelined behind another statement and outside a transaction still drops a state-holding backend, and the client is not told: the native fix above has no libpq equivalent yet. Disconnecting the client instead fails 8 checks of pgsql-discard_all_txn_block-t from #6417, which require the client to stay usable, so that needs a maintainer decision (see the PR comment). These share their cause with the multiplexing flags and need their own issue.

Relation to other issues and PRs

  • #5658 added the feature; #6424 added the local-error poison and end_poisoned_transaction(); #6417 is the DISCARD ALL tracker issue.
  • #6522 (cancel of an idle session) is not part of this PR.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved recovery after backend failures during transactions, including cases where a COMMIT may have completed but its reply was lost.
    • Avoided unsafe recovery when a COMMIT may be unanswered or session-level state cannot be preserved.
    • Preserved session state when rolling back a poisoned transaction; connections are closed when recovery cannot safely preserve that state.
    • Queued client commands until recovery rollback completes so subsequent commands run in the correct order.
    • Made COMMIT on a poisoned session return the same rollback result as ROLLBACK, without an additional warning.
    • Forwarded DISCARD ALL inside explicit transactions and improved handling of session-level advisory locks.