Run onexecute regardless of max_pipeline so begin() works at 0

[reinierlakhan]

Fixes #1210

Cause

execute(q) in src/connection.js returns one && chain that does two unrelated jobs: it decides whether the pool may pipeline another query onto this connection, and it runs the query's onexecute hook as its last term:

      return write(toBuffer(q))
        && !q.describeFirst
        && !q.cursorFn
        && sent.length < max_pipeline
        && (!q.options.onexecute || q.options.onexecute(connection))

begin() sends BEGIN with { onexecute } and relies on that hook to capture the connection, move(c, reserved) and set c.reserved. With max_pipeline: 0 the sent.length < max_pipeline term is always false, so the chain short-circuits before the hook. The connection is never reserved, and the UNSAFE_TRANSACTION guard in CommandComplete rejects every transaction.

Change

Run the hook whenever the query was actually written, independent of pipeline capacity, and keep the return value's meaning ("may the pool pipeline more onto this connection") unchanged:

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

max_pipeline > 0: behaviour is identical. onexecute in begin() (the only caller) returns a truthy value (the assigned c.reserved function), so the old chain was truthy exactly when the new one is, and the hook ran in exactly the cases it runs now (previously it also only ran when write() returned truthy, which written && preserves).

max_pipeline: 0: execute() reserves the connection and returns false, so go() moves it to full. c.reserved is set, so the BEGIN passes the guard. On the BEGIN's ReadyForQuery (status T), connection.reserved() runs, finds the transaction's queue empty and moves the connection back to reserved. The transaction callback only starts after that (it is awaited behind the BEGIN), and each statement it issues goes through begin()'s handler: execute() returns false -> move(c, full); anything issued while in full is pushed onto the transaction's own queries and drained one at a time by c.reserved() on each ReadyForQuery. Ordering is preserved and nothing is pipelined onto the socket, which is the point of the setting.

Tests

Three tests added to tests/index.js, each on a client with { max: 4, max_pipeline: 0 }:

• a single-statement sql.begin(sql => sql\select 1 as x`)returns1`
• two sequential statements in one transaction (set_config(..., true) then current_setting, so it also proves they ran on the same transaction)
• two concurrent statements in one transaction (array form), which exercises the queries queue path

Run in isolation against a plain PostgreSQL 16 through the repo's tests/test.js harness: all three pass with this change; without it the first fails with UNSAFE_TRANSACTION. eslint src tests with the repo config is clean.

The full suite did not run here: tests/bootstrap.js needs psql/createdb/dropdb on PATH and a server it may reconfigure (ssl=on, wal_level=logical, prepared transactions), which this environment does not have. CI should cover it.

I left cjs/, deno/ and cf/ untouched, following the pattern of separate build commits in this repo; happy to regenerate them in this PR if preferred.

[v0idpwn]

@reinierlakhan I had written a similar fix, just PRed it on #1218. I believe there's a regression in your branch that causes another query to end up in the connection that should be reserved for the transaction. I wrote a test reproducing it here

[Ruckus000]

With the default max_pipeline (and with 1), this change lets an unrelated query run inside another caller's transaction.

The PR description says begin()'s onexecute "returns a truthy value (the assigned c.reserved function)". It doesn't: onexecute in src/index.js has a block body with no return, so it returns undefined. On master, that undefined made execute() return falsy for BEGIN, and go() moved the connection to full. With this PR, execute() returns written && ... && sent.length < max_pipeline, which is true for BEGIN. go() then moves the freshly reserved connection back to busy, and the next ordinary query is pipelined onto it while BEGIN is open.

Repro: plain PostgreSQL 16, max: 1. The outside query is issued only after BEGIN has been written to the socket:

import postgres from 'postgres'

for (const max_pipeline of [undefined, 1, 0]) {
  let beginWritten
  const begun = new Promise(r => (beginWritten = r))
  const sql = postgres(process.env.DATABASE_URL, {
    max: 1,
    ...(max_pipeline === undefined ? {} : { max_pipeline }),
    debug: (_, q) => /^begin/i.test(q.trim()) && beginWritten()
  })
  await sql`select 1`
  const tx = sql.begin(async t => (await t`select now()::text as ts, pg_sleep(0.3)`)[0].ts)
  await begun
  const outside = sql`select now()::text as ts`
  const [txTs, [row]] = await Promise.all([tx, outside])
  console.log(`max_pipeline=${max_pipeline ?? 'default'}: outside query ran inside the transaction = ${row.ts === txTs}`)
  await sql.end()
}

now() is the transaction start time, so equal values mean the outside query ran inside the other transaction.

src/connection.js execute()	default	1	0
3.4.9 / master	false	false	throws (Cannot set properties of undefined (setting 'onclose'))
this PR (#1211)	true	true	false
#1218	false	false	false
always call onexecute, return false when present	false	false	false

The #1211 result reproduced 3 out of 3 times. #1218 keeps onexecute's falsy return in the chain, which is what keeps the connection out of busy until BEGIN completes, so it doesn't have this problem. I'd suggest #1218 over this one, or keeping the return falsy whenever onexecute is present.