Repository navigation
sql(postgres): read the rows of a Bind written before its statement's RowDescription as text - #46408
Conversation
… RowDescription as text
|
Warning Review limit reachedYou'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. View limit details
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. Comment |
There was a problem hiding this comment.
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.
|
The job needs a retry, which I cannot do from here. Generated by Claude Code |
What does this PR do?
Fixes wrong values (or a rejected query) from
Bun.SQLon PostgreSQL when a query without parameters is run again before theRowDescriptionof its first run has been read. The bug came in with #45304. It is on main and in no release.What was wrong
A column whose text does not have the length of its binary form rejects the query with
ERR_POSTGRES_INVALID_BINARY_DATAinstead.No proxy is needed to get there.
transform.columnruns while theRowDescriptionis read, before the columns are stored, so a re-run that it starts is bound in the same gap.Cause
Preparedat itsParseComplete.statement.fieldsis filled later, by theRowDescription.Preparedarm ofdispatch_next.write_bindasks for binary per known column. It knows none, so it asks for text.dispatch_nextrecordedResultFormats::Decodable(orRaw) on the request without looking atstatement.fields.DataCellreads the text as binary.Before #45304 both Bind sites had this guard:
f.binary = !statement.fields.is_empty().Fix
dispatch_nextrecordsResultFormats::Textwhen the statement has no columns yet. This is the|| statement.fields.is_empty()inPostgresSQLConnection.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:
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.ParseCompletewhile the query is issued again: objects,values(), andraw()switched tovalues()(theRawarm).transform.column.RowDescriptionstill asks for binary.bun bd test test/js/sql/postgres-bind-wire.test.ts825373492in place of1234USE_SYSTEM_BUN=1cannot 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:
ParseCompleteERR_POSTGRES_INVALID_BINARY_DATAselect 1234 as i, 5678 as jstarted bytransform.column[{ i: 825373492, j: 892745528 }][{ i: 1234, j: 5678 }]Generated by Claude Code