Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughPostgreSQL OID conversion now uses a shared ChangesPostgreSQL encoding flow
Suggested reviewers: Merge Risk: ⚪ Minimal · up to PostgreSQL parameters without binary encoders now use text consistently, preventing binding failures and value corruption while preserving supported binary encodings. The affected paths are covered by wire-level and database round-trip tests, with no current merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status Reproduced on bun 1.4.3 and on a main debug build against PostgreSQL 17, with the default import { SQL } from "bun";
const sql = new SQL({ url, max: 1 });
await sql`create temp table s (r real, n numeric, t time, ia int4[])`;
await sql`insert into s ${sql({ ia: sql.array([1, 2], "INT4") })}`; // 08P01 insufficient data left in message
await sql`insert into s (n) values (${{ toString: () => "19.99" }})`; // 08P01 insufficient data left in message
await sql`insert into s (r) values (${{ toString: () => "1234" }}) returning r::text`; // stores 2.5931515e-09
await sql`insert into s (t) values (${{ toString: () => "12:34:56" }})`; // 22008 time out of range
Rebased onto main after #41889 landed in the same function. The self-review raised two points, both addressed in 97c62ca: the result-column OID mapping now goes through the same |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the format-code and value-encoding sections of the Bind message now both derive from ParamEncoding, so they structurally cannot disagree.
What was reviewed:
- Traced each previously-working tag (bool/int4/float8/timestamp[tz]/bytea/json[b]) through
for_tag/for_value— same wire bytes as before; the string→text downgrade is preserved. param_tagis behaviorally identical to the oldShort::MAXguard (Int4=u32,Short=u16), just without the post-guard.unwrap().- Checked NULL and
iter.next() == Nonein the format-code loop — format code stays binary but the value section writes-1with no bytes, which Postgres accepts regardless of format. - Test file uses
describeWithContainer(local container, no external network),await using, whole-objecttoEqual, and matches the existingpostgres-*.test.tslayout intest/js/sql/.
Extended reasoning...
Overview
This PR fixes a class of Postgres Bind-message bugs where write_bind in src/sql_jsc/postgres/PostgresRequest.rs declared parameters as binary (format code 1) using the decode-side predicate Tag::is_binary_format_supported(), but then encoded numeric/float4/time/int4[]/float4[] values as text — causing 08P01/22008/54000 errors or silent garbage. The fix introduces a ParamEncoding enum enumerating exactly the encoders that exist (Text, Json, Bool, Int4, Float8, Timestamp, Bytea) and drives both the format-code loop and the value-encoding switch from it. The dead int4_array arm (which wrote a single int4, not an array) is removed, param_tag() collapses the duplicated OID→Tag logic, and Tag.rs gets a doc comment clarifying is_binary_format_supported is decode-side only. A new container-backed test file exercises numeric/real/time columns, int4[]/real[] via sql(object) and sql.unsafe(), and round-trips the true binary-encoded types.
Security risks
None. This is client-side wire-protocol encoding for outbound Bind messages; no auth, crypto, permissions, or untrusted-input parsing is touched. The only trust-boundary value here is the server-reported OID, and the new param_tag handles out-of-u16-range OIDs by falling back to text (identical to the prior Short::MAX guard, but without the post-guard .unwrap()).
Level of scrutiny
Medium — protocol encoding is correctness-critical, but the change is narrow and mechanical. I verified behavior preservation for every previously-handled tag: json/jsonb → Json (format 0, JSON.stringify), bool/int4/float8/timestamp/timestamptz/bytea → same binary arms, and the value.is_string() → text downgrade is preserved via for_value. Int4/Short are u32/u16, so Short::try_from fails exactly when the old (Short::MAX as Int4) < parameter_field check did. The NULL path (is_empty_or_undefined_or_null → write -1) is unchanged and is format-code-agnostic per the protocol. The refactor is a direct application of REVIEW.md's "one source of truth" rule — encode/decode were sharing a predicate that only fit decode, and the two Bind sections now cannot diverge.
Other factors
The bug hunter exited dry_streak with no findings. No CODEOWNERS entry covers src/sql/ or test/js/sql/. The new test file follows the established test/js/sql/postgres-*.test.ts per-topic pattern, uses describeWithContainer from harness (local Postgres, no external network), await using for cleanup, whole-object toEqual assertions, and no sleeps or hardcoded ports. The PR description is thorough and honest about scope (interaction with #35508, remaining Date→time and plain-array cases tracked separately, unrelated pre-existing sql.test.ts failures). The timeline shows a single commit with no prior review activity.
|
Updated 6:58 AM PT - Sep 8th, 2026
✅ @robobun, your commit 50144ba0b53d8bb062b24ad4243a85d0858ced4f passed in 🧪 To try this PR locally: bunx bun-pr 41912That installs a local version of the PR into your bun-41912 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…inary encoder write_bind picked each parameter's format code from Tag::is_binary_format_supported(), the list of types DataCell can decode from binary. The encoder only has binary arms for bool, int4, float8, timestamp(tz) and bytea. A parameter the server typed as numeric, real, time or real[] was declared binary but written as String(value), and an int4[] parameter was written as a single int4. The server then read text bytes as the binary representation: 08P01, 22P03 or 22008 errors, or a silently wrong real value. One ParamEncoding enum now drives both the format-code section and the value section of the Bind message. Types without a binary encoder are sent as text and parsed by the server.
… decide each Bind encoding once FieldDescription::type_tag truncated the result-column OID to 16 bits, while DataCell mapped an out-of-range OID to text. A user-defined type whose OID aliases a binary-decoded builtin modulo 65536 was requested in binary and then read as text. Tag::from_oid is now the only mapping, used for Bind parameters, result format codes and DataCell. write_bind now records each parameter's encoding while it writes the format codes and reuses it for the value section, so a getter or toString() that runs between the two sections cannot make them disagree. Adds mock-server tests that pin the Bind bytes for both cases, and a pgDecodeBind helper in wire-frames.ts.
…he loop restructure
b2159f6 to
b42cdb3
Compare
|
Heads-up on overlap in |
…ind bytes with mock-server tests FieldDescription::type_tag() truncated a result column's OID to 16 bits for the Bind result format code, while DataCell mapped an OID above 65535 to text. A user-defined type whose OID aliases a binary-decoded builtin modulo 65536 was requested in binary and read as text. Tag::from_oid() is now the one mapping, for parameters, result format codes and DataCell. wire-frames.ts gains pgNoData() and pgDecodeBind(). The new mock-server tests in postgres-bind-parameter-format.test.ts check the exact format codes and value bytes of the Bind message without a database. The container tests from #41912 and the Date test file from #41955 are carried over.
|
Cross-reference: #41976 fixes the same format-code / encoder mismatch with a single pass over the parameters (the branch that writes binary bytes flips its own format code), and also stops the existing binary arms from coercing a Date, array or object ( |
|
Closing in favor of #41976. It makes the Bind format code follow the encoder that actually runs, which is the same fix as the The parts unique to this PR were carried over there in c2ed427: |
1 participant
Problem
sql({ ids: sql.array([1, 2], "INT4") })into anint4[]column fails with08P01 insufficient data left in message. A decimal.js style object bound tonumericfails with08P01, totimewith22008 time out of range. Bound toreal,"1234"stores2.5931515e-09.write_bind(src/sql_jsc/postgres/PostgresRequest.rs) took each parameter's format code fromTag::is_binary_format_supported(), the decode-side list. Binary encoders only exist for bool, int4, float8, timestamp(tz) and bytea. numeric, float4, time and float4[] were declared binary and written asString(value). Theint4_arrayarm wrote one int4.FieldDescription::type_tag()truncated the OID to 16 bits for the format code, whileDataCellmapped an OID above 65535 to text.Fix
ParamEncodingenum lists the encoders that exist.write_binddecides it once per parameter and writes the format code and the value bytes from it. Types without a binary encoder go out as text, as string values already do.Tag::from_oid()is now the only OID-to-Tagmapping, for parameters, result format codes andDataCell.test/js/sql/postgres-bind-param-format.test.ts(new, mock server plus PostgreSQL 17; 6 of 8 tests fail on bun 1.4.3). Alsosql.test.ts,sql-prepare-false.test.tsand everypostgres-*.test.ts.Background
prepare: true, Bun binds with the types the server reports.Dateandsql.array()values get OID 0, so the server infersnumeric,real,timeorint4[]from the column or cast.DataCelldecodes numeric, float4, time, int4[] and float4[] from binary. Nothing encodes them.Notes
sql({ ia: sql.array([1,2], "INT4") })intoint4[]:08P01/{1,2}sql({ ra: sql.array([1.5,2.5], "REAL") })intoreal[]:54000/{1.5,2.5}{ toString: () => "19.99" }intonumeric:08P01/19.99{ toString: () => "1234" }intoreal: stores2.5931515e-09/1234{ toString: () => "12:34:56" }intotime:22008/12:34:56[1, 2]intoint4[]:08P01/22P02 malformed array literal: "1,2"(postgres: encode array parameters as text array literals #33579 makes plain JS arrays work and builds on the text format)Dateintotime:22008 time out of range/22007 invalid input syntax for type time: "Thu Jan 01 1970 ..."(theDatetext form is a separate issue, see postgres: encode Date and object parameters correctly with prepare: false #39452)sql.array()in a tagged template was not affected:bindParampushes the serialized string and appends a$1::TYPE[]cast, and a string value was already sent as text. Thesql(object)helper andsql.unsafe()bind theSQLArrayParameterobject itself.bool) was requested in binary and then read as text. The mock testa result column whose OID is above 65535 is requested and decoded as textcovers it.toString()or getter that changed another bound value in between could still produce a format/bytes mismatch. A probe with such a value now round-trips.match; whichever lands second needs a small rebase. Rebased on top of sql(postgres): reject a non-BufferSource value bound to a bytea parameter #41889 (bytea type check), which already landed.wire-frames.tsgainspgNoData()andpgDecodeBind()for the mock tests, with a self-test inwire-frames.test.ts.sql.test.tsrun has the same failures with and without this change. They come from the local server setup (SQL_ASCII encoding, no md5/scram roles, prepared transactions disabled, PG 17pg_databaseshape) and debug-build timeouts, not from parameter binding or result decoding.[human-review] gate passed · iteration 0 · 7 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file