Skip to content

Fix sql.begin() sending BEGIN on an unreserved connection - #1218

Open
v0idpwn wants to merge 2 commits into
porsager:masterfrom
v0idpwn:chore/fix-pipeline-boundary-check
Open

v0idpwn wants to merge 2 commits into
porsager:masterfrom
v0idpwn:chore/fix-pipeline-boundary-check

Conversation

@v0idpwn

@v0idpwn v0idpwn commented Sep 9, 2026

Copy link
Copy Markdown

sql.begin() could send BEGIN to the server without reserving the
connection.

Depending on pool size, this errored with UNSAFE_TRANSACTION or
TypeError. The transaction stayed open on the server either way, and
the connection went back to the pool, so unrelated queries then ran
inside it.

With max_pipeline: 0 this error happened consistently. With the default
max_pipeline of 100 the same thing happened only when BEGIN was the
query that filled a connection's pipeline, which is why the failure was
intermittent.

The issue is in execute(): after writing the query it returns an &&
chain that states whether the connection can accept more queries, and
one of its terms is sent.length < max_pipeline. The onexecute hook,
which is how sql.begin() reserves the connection, was the last term of
that chain. Whenever the capacity term was false the chain
short-circuited and the hook never ran, even though BEGIN had already
been written. With max_pipeline: 0 that term is false for every query,
so the hook never ran at all.

Move the hook ahead of the capacity terms so it runs whenever the query
was written. Reserving the connection is a consequence of having sent
BEGIN, unrelated to the query limit in the connection.

Add tests for BEGIN at the pipeline boundary and with max_pipeline: 0.

Fixes #1189 #1210

sql.begin() could send BEGIN to the server without reserving the
connection.

Depending on pool size, this errored with UNSAFE_TRANSACTION or
TypeError.  The transaction stayed open on the server either way, and
the connection went back to the pool, so unrelated queries then ran
inside it.

With max_pipeline: 0 this error happened consistently.  With the default
max_pipeline of 100 the same thing happened only when BEGIN was the
query that filled a connection's pipeline, which is why the failure was
intermittent.

The issue is in execute(): after writing the query it returns an &&
chain that states whether the connection can accept more queries, and
one of its terms is sent.length < max_pipeline.  The onexecute hook,
which is how sql.begin() reserves the connection, was the last term of
that chain.  Whenever the capacity term was false the chain
short-circuited and the hook never ran, even though BEGIN had already
been written.  With max_pipeline: 0 that term is false for every query,
so the hook never ran at all.

Move the hook ahead of the capacity terms so it runs whenever the query
was written.  Reserving the connection is a consequence of having sent
BEGIN, unrelated to the query limit in the connection.

Add tests for BEGIN at the pipeline boundary and with max_pipeline: 0.

Fixes porsager#1189
execute() must keep returning a falsy value for BEGIN so the pool puts
the connection in the full queue rather than busy.  Otherwise a query
issued while BEGIN is in flight is pipelined behind it and runs inside
the transaction.
@Ruckus000

Copy link
Copy Markdown

A production data point for this fix, and for #970 / #1210.

  • Setup: Next.js on Vercel, Supabase Supavisor in transaction mode (port 6543), postgres.js 3.4.8 via drizzle-orm, prepare: false, max: 3.
  • Symptom: a page that issued 8 concurrent queries hung until the platform's 300s timeout, on every load. The query that postgres.js had pipelined onto a busy connection never resolved, and PostgreSQL showed its backend active / wait event ClientRead.
  • Confirmation: raising max to 10, so nothing needed to pipeline, made the page load; that was the confirming test. We then shipped max_pipeline: 0 with max: 3.
  • Patch: we had to patch execute() to run onexecute unconditionally, because otherwise every sql.begin throws (sql.begin() always throws UNSAFE_TRANSACTION when max_pipeline is 0 #1210).

We run that patch in production. Its semantics match this PR: call the hook whenever the bytes were written, and keep a falsy return while it's present. In our local tests it passes concurrent transactions, savepoints and rollback at max_pipeline: 0, and keeps the transaction's connection out of ordinary pipelining at the default and at 1. (See my comment on #1211 for why that last part matters.)

@RotoPL

RotoPL commented Oct 2, 2026

Copy link
Copy Markdown

+1 for this approach — we hit the same bug in our app and landed on
the same semantics independently, so here's a second data point.

Setup: postgres 3.4.8 (code unchanged in 3.4.9), Node 24.13, Drizzle,
Supabase Supavisor transaction mode (port 6543), prepare: false.

Why we need max_pipeline: 0: once the pool saturates, a parameterless
query pipelined onto a busy connection is never answered by the transaction
pooler, and everything queued behind it on that socket wedges with it.
Minimal repro on max: 1: three concurrent sql`select 1 as a` etc. — the
first resolves, the rest never do, and later queries on that connection hang
too. Parameterized queries aren't affected (describeFirst stops them
pipelining), which made it look intermittent for us. (Supabase tracking:
supabase/supabase#50358.)

Then max_pipeline: 0 fails every sql.begin() with UNSAFE_TRANSACTION,
as described in #1210.

Our patch keeps the falsy return for queries carrying onexecute, like
this PR, so the connection is never offered for pipelining while BEGIN is in
flight:

build(q)
const pipelinable = write(toBuffer(q))
  && !q.describeFirst
  && !q.cursorFn
  && sent.length < max_pipeline
return q.options.onexecute
  ? q.options.onexecute(connection)
  : pipelinable

One small difference from this PR: we run the hook even when write()
returns false. As far as we can tell a false write() only means socket
backpressure — the BEGIN is still sent — so skipping the hook there would
reintroduce UNSAFE_TRANSACTION under backpressure, at any max_pipeline.
Worth considering, unless I'm misreading nextWrite.

Results (max: 3, max_pipeline: 0, against a live Supavisor
transaction pooler; read-only): 5 rounds × 41 concurrent operations — 8
sql.begin() transactions each with a nested savepoint, 1 transaction that
throws (rollback), 16 parameterized and 16 parameterless queries each
returning a distinct value. 205/205 resolved with their own values, no stalls,
no UNSAFE_TRANSACTION. Stock 3.4.8 on the same test: stalls at the default
max_pipeline, UNSAFE_TRANSACTION at 0. Same result through Drizzle's
db.transaction() (192 ops incl. rollbacks, 25-way parameterless contention
on max: 10).

Happy to turn this into a test for tests/index.js if useful — though our
verification was against Supavisor, not the plain-Postgres CI setup.

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.

BEGIN can reach PostgreSQL without reserving the transaction connection

3 participants