fix(pg): preserve separator when clamping an over-max LIMIT valu#21019
fix(pg): preserve separator when clamping an over-max LIMIT valu#21019nlimpid wants to merge 3 commits intobytebase:mainbytebase/bytebase:mainfrom nlimpid:fix/pg-limit-offset-trailing-junknlimpid/bytebase:fix/pg-limit-offset-trailing-junkCopy head branch name to clipboard
Conversation
A read query whose LIMIT exceeds the enforced result-row cap (e.g. `select 1 LIMIT 2000 OFFSET 0;` with cap 1000) was rewritten to `select 1 LIMIT 1000OFFSET 0;`, which PostgreSQL 15+ rejects with "trailing junk after numeric literal" (SQLSTATE 42601). rewriteSelectLimit replaced the LIMIT value by splicing the original text using the omni AST byte offsets of the value's A_Const node. omni reports A_Const.Loc.End as the start of the next token, so that span includes the whitespace separating the value from a following clause; the splice dropped that separator and glued the new value onto OFFSET / FOR UPDATE. Values within the cap skip the rewrite, which is why only over-cap limits followed by a clause failed. On PostgreSQL <=14 the lexer split `1000OFFSET` into two tokens, masking the bug. Recompute the literal's real end (integerLiteralEnd) instead of trusting Loc.End so the separating whitespace is preserved. The splice is now also gated on the node being a plain integer constant: non-integer limits (LIMIT $1, LIMIT (1+2), LIMIT 1.5) fall back to the CTE wrapper instead of being partially scanned, and the scanner accepts every integer form PostgreSQL 16 parses (decimal, 0x hex, 0o octal, 0b binary, with underscores such as 1_000 or 0x3_E8) plus a token-boundary check, so clamping can no longer corrupt the value (LIMIT 1_000 -> LIMIT 100_000) or emit invalid SQL (LIMIT 0x3E8 -> LIMIT 100x3E8). Limits already at or below the cap — including LIMIT 0 and negative values — are kept verbatim. Add inline and clause-order tests covering the reported case and isolating variants, and tighten the previously toothless Contains assertions that let the malformed output pass.
…scanning omni reports A_Const.Loc.End as the start of the next token, so trimming trailing whitespace from sql[Loc.Start:Loc.End] yields the literal's real end. This replaces the digit-charset scanner while keeping the separator before OFFSET / FOR UPDATE intact. Keep the (int, bool) contract on integerFromNode so LIMIT 0 stays untouched and non-integer constants still fall back to the CTE wrapper.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45fc539b31
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if loc.Start < 0 || loc.End <= loc.Start || loc.End > len(sql) { | ||
| return 0, false | ||
| } | ||
| literal := strings.TrimRight(sql[loc.Start:loc.End], " \t\r\n\f\v") |
There was a problem hiding this comment.
Preserve comment-only separators after LIMIT literals
When a valid query uses a block comment as the only separator, such as SELECT 1 LIMIT 2000/**/OFFSET 0, the AST span extends to the next token, but TrimRight removes only whitespace. The splice therefore deletes the comment along with the literal and produces LIMIT 1000OFFSET 0, recreating the parse error this change is intended to fix. Determine the integer token's actual end or otherwise preserve all intervening SQL trivia, including comments.
Useful? React with 👍 / 👎.
What
Fix PostgreSQL result-limit rewriting when an existing
LIMITexceeds the configured row cap and is followed by another clause.For example, with a row cap of 1000:
was incorrectly rewritten as:
It is now rewritten as:
The rewrite now derives the integer literal's actual end from the original SQL text instead of using the omni AST
A_Const.Loc.End, which may point to the beginning of the next token and include the separating whitespaceWhy
A_Const.Loc.Endmay include the whitespace between theLIMITvalue and the following clause. Replacing the entire reported span removes that whitespace and joins the replacement value withOFFSETorFOR UPDATE.PostgreSQL 15 and later reject the resulting token, such as
1000OFFSET, with:This only affects queries whose existing
LIMITis greater than the enforced result-row cap because lower limits are kept unchanged.