Skip to content

sql(postgres): read the rows of a Bind written before its statement's RowDescription as text - #46408

Merged
dylan-conway merged 1 commit into
mainfrom
claude/postgres-named-bind-format
Oct 21, 2026
Merged

dylan-conway merged 1 commit into
mainfrom
claude/postgres-named-bind-format

Conversation

@dylan-conway

Copy link
Copy Markdown
Member

What does this PR do?

Fixes wrong values (or a rejected query) from Bun.SQL on PostgreSQL when a query without parameters is run again before the RowDescription of its first run has been read. The bug came in with #45304. It is on main and in no release.

What was wrong

const run = () => sql`select 1234 as n`.execute();
const first = run();
// the client has read ParseComplete, the RowDescription is still on its way
const second = run();
await first;  // [{ n: 1234 }]
await second; // [{ n: 825373492 }], the bytes of "1234" read as a binary int4

A column whose text does not have the length of its binary form rejects the query with ERR_POSTGRES_INVALID_BINARY_DATA instead.

No proxy is needed to get there. transform.column runs while the RowDescription is read, before the columns are stored, so a re-run that it starts is bound in the same gap.

Cause

  • A named statement without parameters becomes Prepared at its ParseComplete. statement.fields is filled later, by the RowDescription.
  • A request for the same query that is dispatched in between takes the Prepared arm of dispatch_next.
  • write_bind asks for binary per known column. It knows none, so it asks for text.
  • dispatch_next recorded ResultFormats::Decodable (or Raw) on the request without looking at statement.fields.
  • When the rows arrive the columns are known, and DataCell reads the text as binary.

Before #45304 both Bind sites had this guard: f.binary = !statement.fields.is_empty().

Fix

dispatch_next records ResultFormats::Text when the statement has no columns yet. This is the || statement.fields.is_empty() in PostgresSQLConnection.rs. The request flag then says what its Bind asked for, as its doc comment states. The bytes on the wire do not change.

Not affected:

  • A statement with parameters. Its first request sends Parse + Describe + Sync and holds the queue until the ReadyForQuery.
  • prepare: false. It always asks for text.
  • .raw(). It hands out the bytes without decoding them.

How did you verify your code works?

Four new tests in test/js/sql/postgres-bind-wire.test.ts. Their mock sends the column in the format that each Bind asked for, as PostgreSQL does.

  • Three hold back everything behind the first ParseComplete while the query is issued again: objects, values(), and raw() switched to values() (the Raw arm).
  • One starts the re-run from transform.column.
  • Each also pins the result formats of the Binds. The run after the RowDescription still asks for binary.
build bun bd test test/js/sql/postgres-bind-wire.test.ts
debug build of main (491aaad) 57 pass, 4 fail, each with 825373492 in place of 1234
debug build with the fix 61 pass

USE_SYSTEM_BUN=1 cannot show the failure, because the bug is in no release. The failing run above is a debug build of main without the fix.

I also ran both cases against PostgreSQL 16.15 with int4, bool, float8, timestamptz, bytea and text columns:

case main with the fix
a proxy that changes no byte and only splits the backend stream behind the first ParseComplete re-run rejects with ERR_POSTGRES_INVALID_BINARY_DATA all three runs return the same correct row
no proxy, re-run of select 1234 as i, 5678 as j started by transform.column [{ i: 825373492, j: 892745528 }] [{ i: 1234, j: 5678 }]

Generated by Claude Code

@coderabbitai

coderabbitai Bot commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 7 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 221ab843-37b0-423e-acab-4bfa2ad7f480
📥 Commits

Reviewing files that changed from the base of the PR and between 0793250 and b9eba13.

📒 Files selected for processing: 2
  • src/sql_jsc/postgres/PostgresSQLConnection.rs
  • test/js/sql/postgres-bind-wire.test.ts
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Oct 21, 2026 •

Copy link
Copy Markdown
Collaborator
Updated 5:05 PM PT - Oct 21st, 2026

@claude, your commit b9eba13 is building: #130442

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline finding, I also checked the other sites that record ResultFormats for the same gap: the Pending arm (line 1977) and the invalidated-statement retry at line 2713 both already record Text, and write_bind derives the per-column format codes from result_fields itself, so the wire bytes already matched and only the request's flag was out of step.

Extended reasoning...

The change is a one-condition widening in the Prepared arm of the Postgres queued-request flush path in src/sql_jsc/postgres/PostgresSQLConnection.rs plus four mock-server tests in test/js/sql/postgres-bind-wire.test.ts; it touches no auth, crypto, or injection surface. The sibling ResultFormats sites and the Bind writer were read to confirm the fix covers the whole class; the inline finding about the transform.column re-entry path is the reason a human should still look.

Copy link
Copy Markdown
Member Author

buildkite/bun (#130442) is red from one job, darwin aarch64 - test-bun, and the failure is not from this change.

  • What failed: the job stopped before any test ran. On agent darwin-aarch64-26.6.1-1 the download of the build timed out: buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun' (log).
  • Same on main: builds #130439 and #130420 fail with the same error on the same agent.
  • The rest of this build: the other 177 finished jobs passed. test/js/sql/postgres-bind-wire.test.ts ran in passing shards on Debian x64, x64-asan and aarch64, Ubuntu x64 and aarch64, Alpine x64 and aarch64, and Windows x64 and aarch64. The other darwin shards had not finished when I wrote this.
  • No fix to port: I found no open PR for it. The abort on a timed-out download in scripts/runner.node.ts is deliberate, so this looks like a problem with that agent.

The job needs a retry, which I cannot do from here.


Generated by Claude Code

@dylan-conway
dylan-conway merged commit 49b80f5 into main Oct 21, 2026
10 of 11 checks passed
@dylan-conway
dylan-conway deleted the claude/postgres-named-bind-format branch October 21, 2026 23:58
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.

3 participants