porsager / porsager/postgres

BEGIN can reach PostgreSQL without reserving the transaction connection

Open
#1,189 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
JavaScript
Stars
8.7k
Forks
374
Avg merge
11d 16h
Merged PRs (30d)
1

Description

Summary

In postgres@3.4.9, a transaction's BEGIN can be written to the socket while its onexecute(connection) reservation hook is skipped.

The affected ordering is currently present on master in both Node entry points:

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

At the pipeline boundary, adding BEGIN to sent makes sent.length === max_pipeline, so short-circuit evaluation skips onexecute even though the protocol bytes were accepted. sql.begin() then has no reserved connection, while PostgreSQL has already entered a transaction.

This is related to #823 and the closed, unmerged #1159. Those identify the intermittent UNSAFE_TRANSACTION/pipeline symptom. The deterministic case below additionally demonstrates the server-side isolation impact when max: 1: the connection returns to ordinary use as idle in transaction, and the next unrelated query runs on the same backend.

Environment

  • postgres@3.4.9 from the npm tarball, unmodified
  • Node.js 24 (the ESM case was included in the private disclosure; the equivalent CJS case was subsequently reproduced as well)
  • PostgreSQL 17
  • Reproduced repeatedly at the exact pipeline boundary

Reproduction scope

The reproduction below deterministically isolates the exact-pipeline-boundary trigger. It still performs the real socket write; forcing the wrapper to return true only removes socket backpressure from this case.

A separate trigger exists when the native socket.write() returns false after accepting the bytes: the same short-circuit expression skips onexecute before reaching the pipeline check. That backpressure path is supported by source inspection but is not independently reproduced by the script below.

Minimal reproduction

Run against a disposable database:

DATABASE_URL=postgres://... node repro-transaction-reservation.mjs
import assert from 'node:assert/strict'
import net from 'node:net'
import postgres from 'postgres'

const databaseUrl = process.env.DATABASE_URL
if (!databaseUrl) throw new Error('Set DATABASE_URL to a disposable PostgreSQL database')
const endpoint = new URL(databaseUrl)

async function connectSocket() {
  const socket = net.createConnection({
    host: endpoint.hostname,
    port: Number(endpoint.port || 5432),
  })
  await new Promise((resolve, reject) => {
    socket.once('connect', resolve)
    socket.once('error', reject)
  })

  // Preserve the real write, but make this boundary deterministic.
  const nativeWrite = socket.write
  socket.write = function acceptedWrite(chunk, ...args) {
    Reflect.apply(nativeWrite, this, [chunk, ...args])
    return true
  }
  return socket
}

const sql = postgres(databaseUrl, {
  fetch_types: false,
  max: 1,
  max_pipeline: 1,
  prepare: false,
  socket: connectSocket,
})
const observer = postgres(databaseUrl, { max: 1, prepare: false })

// Stock 3.4.9 can emit a secondary rejection after the primary begin rejection.
const onUnhandled = () => {}
process.on('unhandledRejection', onUnhandled)

try {
  await sql`select 1`

  const active = Promise.resolve(sql`
    select pg_backend_pid()::integer as pid
    from pg_sleep(0.25)
  `.execute())

  const transaction = Promise.resolve(sql.begin(async () => true))
  const transactionObserved = transaction.then(
    () => ({ status: 'fulfilled', error: null }),
    (error) => ({ status: 'rejected', error }),
  )

  const [{ pid }] = await active
  const transactionResult = await Promise.race([
    transactionObserved,
    new Promise((resolve) => setTimeout(
      () => resolve({ status: 'still_pending', error: null }),
      1500,
    )),
  ])

  const [activity] = await observer.unsafe(
    'select state from pg_stat_activity where pid = $1::integer',
    [pid],
  )

  const [{ reusedPid }] = await sql`
    select pg_backend_pid()::integer as "reusedPid"
  `

  console.log({
    transactionStatus: transactionResult.status,
    transactionError: transactionResult.error?.message ?? null,
    backendPid: pid,
    backendState: activity?.state,
    nextQueryBackendPid: reusedPid,
  })

  assert.equal(transactionResult.status, 'rejected')
  assert.match(transactionResult.error?.message ?? '', /undefined/)
  assert.equal(activity?.state, 'idle in transaction')
  assert.equal(reusedPid, pid)
} finally {
  await Promise.allSettled([
    sql.end({ timeout: 0 }),
    observer.end({ timeout: 1 }),
  ])
  await new Promise((resolve) => setImmediate(resolve))
  process.off('unhandledRejection', onUnhandled)
}

Observed repeatedly:

{
  transactionStatus: 'rejected',
  transactionError: "Cannot set properties of undefined (setting 'onclose')",
  backendPid: <pid>,
  backendState: 'idle in transaction',
  nextQueryBackendPid: <same pid>
}

Impact

The next logical caller can execute on a physical connection that is still inside the orphaned transaction. A later COMMIT, ROLLBACK, disconnect, or transaction timeout can therefore commit or roll back work belonging to a different caller.

This is a data-integrity and cross-request isolation issue, not only a failed transaction callback.

Expected behavior

For a query carrying options.onexecute:

  • once its protocol bytes have been accepted by write() without throwing, reserve the connection exactly once;
  • do not make reservation conditional on sent.length < max_pipeline;
  • treat socket.write() === false as accepted with backpressure, not as unsent;
  • after reservation, do not offer the connection to ordinary pipeline work;
  • preserve the existing describe-first and cursor behavior;
  • cover both ESM and CJS at the exact pipeline boundary and under backpressure.

I can prepare a narrowly scoped PR and regression tests if that would help.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Inspect the two Node entry points containing the shown write/onexecute expression, then run the supplied reproduction against a disposable PostgreSQL database. Add regression coverage for the exact pipeline boundary and backpressure paths in both ESM and CJS. Done means accepted transaction bytes reserve the connection once and ordinary work cannot reuse it while the transaction remains active.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, nodejs, postgresql
Domain
backend, databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.