Send transaction control on the simple protocol so prepare: true never names commit - #1212
Open
mirhet wants to merge 1 commit into
Open
Send transaction control on the simple protocol so prepare: true never names commit#1212mirhet wants to merge 1 commit into
prepare: true never names commit#1212mirhet wants to merge 1 commit into
Conversation
With `prepare: true`, `begin()` sent `savepoint`, `rollback to`, `rollback`, `commit` and `prepare transaction` as tagged templates, so each became a named prepared statement cached on the client connection. On a transaction pooler that does not track named statements (or after the pooler evicts one), the Bind of a named `commit` reaches a backend that never parsed it and fails with SQLSTATE 26000. That error aborts the transaction, and `FetchPreparedStatement` is in `retryRoutines`, so the driver re-sends `commit` on the aborted transaction. Postgres answers that COMMIT with ROLLBACK and no error, and the transaction's writes are lost silently. Reproduced against a Supavisor pooler in transaction mode with one client connection and a second client keeping the other backends busy, twenty sequential `sql.begin` inserts, body statement unnamed: before: 9 of 20 rows, 31 `commit` sends, 0 errors after: 20 of 20 rows, 20 `commit` sends, 0 errors `begin` already goes through `unsafe`. This sends the other five control statements the same way. A zero-argument `unsafe` is `simple: true`, so no `prepare` setting can name it, and the transaction lifecycle (connection close, release only when idle, per-query error capture) is unchanged. Identifier quoting matches `sql(name)`; the `prepare transaction` name doubles single quotes instead of being spliced in raw.
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.
Problem
With
prepare: true,begin()sendssavepoint,rollback to,rollback,commitandprepare transactionas tagged templates on the scoped client. A tagged template takes the pool'sprepareoption, so each of them becomes a named prepared statement cached per client connection.On a transaction pooler that does not track named statements (Supavisor, or pgbouncer with
max_prepared_statements = 0, or any pooler after it evicts a statement), theBindof a namedcommitcan reach a backend that never parsed it. Postgres answers SQLSTATE 26000 and aborts the transaction.FetchPreparedStatementis inretryRoutines, so the driver re-sendscommiton the aborted transaction. Postgres turns thatCOMMITinto aROLLBACKwith no error, thebegin()promise resolves, and the transaction's writes are gone.beginitself already goes throughunsafe, so only the other five control statements were exposed.Change
The five control statements go through a zero-argument
unsafe, likebegin. A zero-argumentunsafeissimple: true, so nopreparesetting can name it. The rest of the transaction lifecycle (connection close, release only when idle, per-query error capture) is untouched.Identifier quoting for
savepoint/rollback tomatchessql(name). Theprepare transactionname now doubles single quotes instead of being spliced in throughsql.unsafe(prepare)inside the template.cjs/,deno/andcf/are thenpm run buildoutput forsrc/index.jsonly.Reproduction
Supavisor in transaction mode. One client with
prepare: true, max: 1runs twenty sequentialsql.begininserts; a second client (prepare: false, max: 6) keeps the other backends busy so the pooler hands the first client a different backend each time. The insert goes throughunsafe(text, [i]), so the body is unnamed and onlycommitis named.commitsends counted with thedebughook.Two runs each, same numbers. The eleven extra
commitsends on master are the retries; the eleven missing rows are the transactions those retries rolled back.probe.mjs
Not covered
The test suite runs against a plain Postgres, which has no pooler and cannot show the loss, so there is no new test. The statements inside the transaction body are still named under
prepare: true; on a non-tracking pooler those fail with a visible 26000 error, which is the documented reason to useprepare: falsethere. This change only removes the case where the failure is silent.