Skip to content

fix: number bare ? placeholders at parse time, in text order (found by SQE) - #712

Open
dpsiderius wants to merge 2 commits into
mainfrom
fix/bare-param-numbering-text-order
Open

dpsiderius wants to merge 2 commits into
mainfrom
fix/bare-param-numbering-text-order

Conversation

@dpsiderius

Copy link
Copy Markdown
Contributor

What was wrong

UPDATE t SET v = ? WHERE k = ? bound its two values in the wrong order. It
matched no row and returned Ok(0) — which is also the rows-affected value an
optimistic-concurrency check reads as "someone else won the race", so a
compare-and-swap built on it failed 100% of the time while looking like
ordinary contention. Nothing errored, and param_count correctly said 2.

Separately, SELECT <index columns only> FROM t WHERE a = ? AND b = ? on a
table with a usable index reported statement wants 1 parameter(s) but 2 were bound.

These were the same bug. The index for a bare ? was assigned during code
generation, from a next_param counter on RegAlloc. That makes the index a
property of compilation rather than of the SQL text, and compilation breaks
that two ways:

  • Codegen does not visit expressions in text order. compile_update
    compiles the WHERE operand before the SET assignments, because the scan
    must be positioned before the row body is emitted
    (src/codegen/stmt/update.rs:186). So the WHERE placeholder took index 1
    and the SET placeholder took 2.
  • There is more than one RegAlloc per statement. Eight sites call
    RegAlloc::new(), each starting the counter at zero. The covering-index seek
    path compiles through a second one, so numbering restarted mid-statement and
    both placeholders became index 1 — hence param_count = max = 1.

So the general statement is broader than either symptom: bare ? numbering
depended on visit order and plan shape, for every statement type.
These two
are the shapes that happened to surface.

The fix

Assign the index in the parser, in text order, and carry it on
ParamKind::Anonymous(u32). That is where SQLite assigns it
(sqlite3ExprAssignVarNumber) and why its numbering cannot depend on the plan.
Parser holds one high-water mark, per-statement by construction because every
parse entry point builds its own Parser for one statement's tokens. ?NNN
raises the mark and a following bare ? continues past it.

RegAlloc::anonymous_param and RegAlloc::numbered_param are deleted along
with the field they mutated, being dead afterwards.

ADR-0044 records why the alternative — a numbering pass between parse and
codegen — was rejected: it needs a visitor reaching every expression position
in every statement type, and a position it misses is this same bug, silently,
in a shape nobody has tested. Assigning at parse makes "was this numbered?"
unrepresentable rather than merely tested.

Why this survived

Explicit ?NNN was never affected, and every existing parameter test is
written with ?1/?2
— the form this repository's own code writes. Drivers
emit bare ?. tests/corpus/api_oracle_test.rs::parameterised_writes_match_the_oracle
on #705 used ?1/?2 throughout, so it was testing our dialect rather than a
consumer's.

Found by SQE driving the embedding API with sqlx-shaped SQL, not by our own
suite. That is the part worth remembering.

Tests

tests/unit/param_numbering_test.rs, 8 tests, asserting on the AST rather
than on a compiled program — deliberately, so the property is pinned where it
is now decided and no future plan can reintroduce the divergence. Covers
UPDATE/SELECT/INSERT/DELETE left-to-right order, the ?NNN high-water
interaction, a repeated ?NNN counting once, and per-statement reset.

All 8 fail under a mutant that collapses every bare ? to index 1.

Verified end to end against the reporter's exact statements, with this commit
cherry-picked onto #705's facade branch:

(1) UPDATE bare ?     -> Ok(1), v = "b"                  [was Ok(0), "a"]
(1) compare-and-swap  -> miss Ok(0), hit Ok(1)           [was Ok(0) always]
(2) covering index    -> param_count=2, 1 row            [was param_count=1, Err]

Gates

make test all green, make test-corpus 387/387 (unchanged from main's
baseline), make lint exit 0 on both clippy passes, make check-mod-files
clean. make check-mvl-limit not run locally (cargo-mvl-limit not installed);
no lifetimes, dyn or unsafe added, so CI should confirm.

Relationship to #705

Independent, and #705 does not need to wait. This is a pre-existing engine
bug, not something the facade introduced — but the facade is what made it
reachable, since there was no writable parameterised entry point before it.

One trivial conflict to expect: this adds ADR-0044 and #705 adds ADR-0043, so
.openspec/adr/index.md conflicts on adjacent lines. Whichever merges second
takes both entries.

Note also that #705's spec 013 Requirement 1 presents rows-affected as a
reliable conflict signal. That was not true for bare ? until this lands.

spend: small, matched.

Refs: 013/Req-1, 013/Req-3, #705

🤖 Generated with Claude Code

dpsiderius and others added 2 commits September 10, 2026 16:06
`UPDATE t SET v = ? WHERE k = ?` bound its two values backwards, matched
nothing, and returned `Ok(0)`. That is also the rows-affected value an
optimistic-concurrency check reads as "someone else won the race", so a
compare-and-swap built on it failed every time while looking like ordinary
contention. On a table with a usable index, a projection of index columns
only collapsed both placeholders onto index 1 and reported wanting one
parameter when it had two.

Both were the same cause. The index for a bare `?` was assigned during code
generation, from a counter on `RegAlloc`, which makes it a property of
compilation rather than of the SQL text. Compilation breaks that two ways:
`compile_update` compiles the `WHERE` operand before the `SET` assignments,
so the numbering came out reversed; and eight sites call `RegAlloc::new()`,
each starting the counter at zero, so a plan that compiles part of a
statement through a second allocator restarts numbering mid-statement.

Assign the index in the parser instead, in text order, and carry it on
`ParamKind::Anonymous(u32)` — which is where SQLite assigns it
(`sqlite3ExprAssignVarNumber`) and why its numbering cannot depend on the
plan. `Parser` holds one high-water mark, per-statement by construction
because every parse entry point builds its own `Parser`. `?NNN` raises the
mark and a following bare `?` continues past it. `RegAlloc`'s two parameter
methods and the field they mutated are deleted, being dead afterwards.

Explicit `?NNN` was never affected, which is why this survived: the existing
parameter tests are written with `?1`/`?2`, the form this repository's own
code writes. Drivers emit bare `?`. `tests/unit/param_numbering_test.rs`
asserts the property on the AST rather than on a compiled program, so no
future plan can reintroduce it; all 8 of its tests fail if the numbering is
collapsed.

Found by SQE driving the embedding API with sqlx-shaped SQL. ADR-0044
records why the alternative — a numbering pass between parse and codegen —
was rejected: it needs a visitor reaching every expression position in every
statement type, and a position it misses is this same bug, silently.

Spend: small, matched.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Introduces an `[Unreleased]` heading, which this file did not have. The
versioning policy above it is about which *minor* version a completed plan
phase ships as; it says nothing about where a fix lands before its version
is cut, and Keep a Changelog — which the file's own header says it follows —
answers that with `[Unreleased]`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.

1 participant