Skip to content

Send transaction control on the simple protocol so prepare: true never names commit - #1212

Closed
mirhet wants to merge 1 commit into
porsager:masterfrom
mirhet:transaction-control-simple-protocol
Closed

mirhet wants to merge 1 commit into
porsager:masterfrom
mirhet:transaction-control-simple-protocol

Conversation

@mirhet

@mirhet mirhet commented Sep 2, 2026 •

Copy link
Copy Markdown

Problem

With prepare: true, begin() sends savepoint, rollback to, rollback, commit and prepare transaction as tagged templates on the scoped client. A tagged template takes the pool's prepare option, 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), the Bind of a named commit can reach a backend that never parsed it. Postgres answers SQLSTATE 26000 and aborts the transaction. FetchPreparedStatement is in retryRoutines, so the driver re-sends commit on the aborted transaction. Postgres turns that COMMIT into a ROLLBACK with no error, the begin() promise resolves, and the transaction's writes are gone.

begin itself already goes through unsafe, so only the other five control statements were exposed.

Change

The five control statements go through a zero-argument unsafe, like begin. A zero-argument unsafe is simple: true, so no prepare setting 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 to matches sql(name). The prepare transaction name now doubles single quotes instead of being spliced in through sql.unsafe(prepare) inside the template.

cjs/, deno/ and cf/ are the npm run build output for src/index.js only.

Reproduction

Supavisor in transaction mode. One client with prepare: true, max: 1 runs twenty sequential sql.begin inserts; 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 through unsafe(text, [i]), so the body is unnamed and only commit is named. commit sends counted with the debug hook.

master: 20 transactions, 9 rows,  31 commit sends, 0 errors
branch: 20 transactions, 20 rows, 20 commit sends, 0 errors

Two runs each, same numbers. The eleven extra commit sends on master are the retries; the eleven missing rows are the transactions those retries rolled back.

probe.mjs
import postgres from './src/index.js'
let commits = 0
const sql = postgres(process.env.DATABASE_URL, { prepare: true, max: 1, debug: (_, q) => { /^commit/i.test(q) && commits++ } })
const churn = postgres(process.env.DATABASE_URL, { prepare: false, max: 6 })
let stop = false
const churner = (async () => { while (!stop) await Promise.all(Array.from({ length: 6 }, () => churn.begin(s => s`select pg_sleep(0.02)`))) })()
const t = 'pgjs_probe_' + Math.random().toString(36).slice(2, 8)
await sql.unsafe(`create table ${t} (i int)`)
let errors = 0
for (let i = 0; i < 20; i++) {
  await new Promise(r => setTimeout(r, 30))
  try { await sql.begin(async sql => { await sql.unsafe(`insert into ${t} values ($1)`, [i]) }) } catch (e) { errors++; console.log('error', e.code, e.message) }
}
stop = true; await churner
const [{ n }] = await sql.unsafe(`select count(*)::int n from ${t}`)
await sql.unsafe(`drop table ${t}`)
console.log({ transactions: 20, rows: n, commitsSent: commits, errors })
await sql.end(); await churn.end()

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 use prepare: false there. This change only removes the case where the failure is silent.

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.
@elucid

elucid commented Oct 6, 2026

Copy link
Copy Markdown

We hit this in prod behind Cloudflare Hyperdrive (origin is non-pooled Neon endpoint) and ended up with an almost identical patch which we're running via pnpm patch on 3.4.8.

We get about ~500/day of prepared statement "…" does not exist (SQLSTATE 26000), consistently on the 3rd/4th statement prepared on the connection i.e COMMIT.

If it's useful I can contribute the test our patch uses. The 26000-on-COMMIT path reproduces on plain Postgres with no pooler: issue deallocate all from inside the transaction right before the callback returns. On master begin() resolves and the row is gone; with this PR it persists. We also have cases for ROLLBACK after deallocate, quoted savepoint names, and a pg_prepared_statements check that application queries stay prepared while the transaction controls don't. Happy to open a PR against your branch.

@pauldraper

pauldraper commented Oct 25, 2026 •

Copy link
Copy Markdown

This seems a bit confusing.

prepare: true means you want prepared statements, but then prepared statements aren't supported by your middleware? Why have prepare: true?

EDIT: It good to prevent the silent data loss I suppose. Event if the setup is bad.

@mirhet

mirhet commented Oct 27, 2026

Copy link
Copy Markdown
Author

Fair point. Our production pooler tracks prepared statements, so prepare: true is valid there. The risk is a pooler without tracking, a wrong config, or an evicted statement. In those cases other queries fail with a visible 26000 error, but the retried commit turns into a ROLLBACK with no error. This PR makes that failure loud. It does not try to make a bad setup work.

@porsager

Copy link
Copy Markdown
Owner

Thanks! This is part of v4 (06cf284), so I am closing it here. The changelog has the details: https://github.com/porsager/postgres/blob/master/CHANGELOG.md

@porsager porsager closed this Oct 29, 2026
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