Repository navigation
fix(postgres): carry the with clause into a probe that reads it - #58
Merged
Merged
Conversation
A probe of JSON dropped the statement's whole WITH clause when any query in it held a placeholder, so one reading a WITH query was refused and its fields came back unknown. Such a probe now carries the clause with its placeholders, pinned to their types, and numbers them from 1.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
There was a problem hiding this comment.
ℹ️ No blocking issues. Two small suggestions inline.
Reviewed changes
Reviewed the one commit that sends the WITH clause with Postgres probes that read it, renumbers pinned placeholders and blocks JSON shapes for writes whose target has the same name as a WITH query.
- Conditional WITH on probes:
grouped_select_textattaches the WITH clause only whenreads_withfinds an unqualified relation named after one of its queries. The clause keeps its$n, and only the probe body after it haswithout_placeholdersapplied. The text is split withstrip_suffix, and if the split fails, every placeholder is replaced withNULL. - Dense renumbering in
Engine::pin: the probe's placeholders are numbered from 1 in sorted order, and each cast is still looked up by its original number.pinonly feeds probe and prewarm describes (postgres.rs:939,postgres.rs:1520), sosettleandanalyzestill read the original numbering, as they did before. target_named_by_withguard: if an INSERT, UPDATE, DELETE or MERGE writes a target that has the same unqualified name as a WITH query, the statement gets no JSON shapes. The probe would otherwise read the WITH query in place of the table.- Oracle cases: new cases in
large_queriesandjson_subqueries, plus re-blessed lines ininference_precisionandpg18_syntax. Each changed line goes fromunknownto a concrete type, or tojsonfor the case where the target has a WITH query's name.
claude-opus-5-5 ｜ 𝕏
Contributor
There was a problem hiding this comment.
✅ No new issues found. Both earlier comments are addressed.
Reviewed changes
This pass covered adec33e, the commit pushed after the previous Pullfrog review.
- Leaf plan guard:
target_named_by_withis nowpub(crate), andinfer::plan_herereturnsPlan::default()when it holds. A RETURNING leaf probe can no longer read a WITH query in place of the table it writes. Returning the empty plan for the whole statement, not just dropping the leaves, is the right call.WithsandLevel::ofbehindRowswould also resolve the target's name to the WITH query. - Oracle: added
returning_leaf_target_named_by_with.sql, now typede: text?.returning_target_named_by_with.sqlmoves fromj: jsontoj: json?. That is less precise because the empty plan also drops the claim thatjsonb_build_objectis non-null, but it is still sound. - Nit: removed the extra blank line in
postgres.rstests.
The PR description still lists returning_target_named_by_with.sql as "now j: json". After this commit it is j: json?.
claude-opus-5-5 ｜ 𝕏
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A JSON probe dropped the statement's whole WITH clause whenever any query in it held a placeholder, since a probe binds nothing. A probe that read one of those queries then named a relation that did not exist (the server answered
relation "monthly" does not exist), and the JSON built over the query's columns came back withunknownfields. Short statements whose WITH queries held no placeholder were typed, which made it look like a large-statement problem.mainlarge_queries/json_over_with_param_short.sql:with recent as (… where org_id = :org_id) select u.id, (select json_agg(json_build_object('at', r.placed_at)) from recent r …)recent: [{ at: unknown? }]?recent: [{ at: timestamptz }]?large_queries/json_over_with_param.sql: four WITH queries and two JSON columns, one over a WITH querymonthly_spend: [{ month: unknown?, cents: unknown? }]?[{ month: timestamptz, cents: int4 }]?pg18_syntax/returning_with_query_param.sqlj: { now: unknown?, was: unknown? }j: { now: text?, was: text? }inference_precision/whole_row_beside_with.sql:jsonb_agg(g)of a data-modifying WITH query's rowgone: [unknown?]gone: [record?]jsonshape::select_text, which the leaf probe of an expression column goes through too). Its placeholders stay, and the describe pins each to the type the statement's own describe gave it; only the placeholders outside the clause are writtenNULL, as before. The probe of a WITH query read under a leaf probe is then the same text as the statement's own probe of it, settled once. With the placeholders writtenNULL, it was another statement to settle, spending the budget twice, and a column selecting a placeholder came out nullable there. A probe that reads none of the queries goes without the clause and binds nothing, so it is still described in the first round.Engine::pin): the server cannot type$2in a statement that never mentions$1, which a probe reading only some of the statement's parameters would be.with users as (select :n::int as name) update users … returning jsonb_build_object('n', name)writes the table, while a probe carrying the clause would read the query by that name and typenasint4(json_subqueries/returning_target_named_by_with.sql, nowj: json). Before this PR the same misreading happened wherever the clause held no placeholder.Checked
--strict-coverage; the whole workspace builds with no warning.packages/postgres/engine.wasmrebuilt (with the other two in one run; only this one changes);nub run typecheckandnub run testagainst Postgres 18.