Skip to content

feat: rows-changed counter, flagged by codegen rather than counted by the opcode (#692) - #694

Open
dpsiderius wants to merge 1 commit into
mainfrom
feat/692-rows-changed-counter
Open

feat: rows-changed counter, flagged by codegen rather than counted by the opcode (#692)#694
dpsiderius wants to merge 1 commit into
mainfrom
feat/692-rows-changed-counter

Conversation

@dpsiderius

Copy link
Copy Markdown

Summary

Nothing in src/vdbe/ reported how many rows an INSERT/UPDATE/DELETE
changed. Spec 013 calls this the one item on its list a consumer cannot
work around
: execute_transaction_step returns rows and the autocommit
flag, so a caller cannot tell an UPDATE that matched from one that did not,
and that distinction is what every optimistic-concurrency scheme is built on.

SQE swaps a table's metadata pointer with a conditional UPDATE and treats
zero rows affected as a lost race. Without the count that becomes
SELECT-then-UPDATE inside a transaction — sound only while the consumer
guarantees a single writer, and every consumer reinvents it.

The obvious implementation is wrong, and measurably so

Counting in the Insert/Delete handlers reports this, per changed row, on
the tree at 0.18.10:

statement opcodes emitted naive count
INSERT Insert 1 ✓
DELETE Delete 1 ✓
UPDATE, single-pass Delete + Insert (update.rs:603,605) 2
UPDATE, two-pass range-seek ephemeral Insert (update.rs:276) + Delete + Insert 3

The two-pass plan is #666/#675's range-seek path, which stashes matched
rowids in an ephemeral b-tree using the same Opcode::Insert. So the same
UPDATE reports 2 or 3 depending on which plan the optimizer picked
, and
neither is 1. Index maintenance is the same shape in reverse: IdxInsert,
IdxDelete and AutoIndexInsert are writes adjacent to a row that are not a
row change.

The opcode does not carry enough information to answer. Codegen does.

Design: OPFLAG_NCHANGE on P5, as SQLite does it

Codegen marks the one mutation that is the row change, with a P5 bit —
stock SQLite's OPFLAG_NCHANGE, same value (0x01), same job. Following it
keeps our opcode semantics aligned with the thing we are a replication of.

P5 was unread by both cursor::insert and cursor::delete, so the bit
was free on exactly the two opcodes that needed it. No new opcode, so the
frozen-set ADRs (0015/0018/0020) stay closed. An UPDATE flags its Insert
and not the paired Delete: one changed row, counted once.

Some(0) and None are different answers

StepOutcome::changes is Option<u64>:

  • Some(0) — "this was an INSERT/UPDATE/DELETE and it changed
    nothing." The lost race. This is the case the requirement exists to make
    visible.
  • None — "not that kind of statement", so a connection tracking
    sqlite3_changes() leaves its stored count alone rather than zeroing it.

Program::counts_changes() is the discriminator, and it is deliberately
static — it asks whether the program contains a flagged instruction,
not whether one executed. An UPDATE whose WHERE matches nothing never
runs its flagged Insert but must still report Some(0).

ADR-0042 records why collapsing this to a bare u64 would be a bug rather
than a simplification: it merges "changed nothing" into "not a counting
statement", and then a SELECT zeroes a count that should have survived it.

No call-site churn

execute_transaction_step becomes a wrapper over
execute_transaction_step_counted, which returns the count. Same pattern
ADR-0040 settled for streaming: one loop, the older signature expressed in
terms of the newer one, so they cannot drift. Its ten existing call sites
across src/bin/, tests, benches and examples are untouched.

Both wrong designs are mutation-checked, not just argued

mutation result
count unconditionally in the handlers fails update_of_one_row_reports_one_under_both_plans, index_maintenance_does_not_count, conditional_update_reports_match
additionally flag UPDATE's Delete fails the same three, and the oracle diff with 4 against the oracle's 2

The second one is the useful evidence that the oracle test is really talking
to the oracle rather than skipping green.

Test plan

  • tests/unit/vdbe_changes_test.rs — 7 tests. The load-bearing one is
    update_of_one_row_reports_one_under_both_plans, which builds both
    plans (perf: UPDATE range-seek always pays for a two-pass deferred-rowid plan it usually doesn't need #675's rule: two-pass when SET touches the scanned index,
    single-pass when it does not) and asserts 1 from each
  • tests/corpus/changes_oracle_test.rs — a thirteen-statement sequence
    diffed against the pinned 3.53.4 oracle's own changes(): both
    UPDATE plans, a miss, a partial DELETE, a full one, and a
    DELETE on an already-empty table
  • cargo test --locked1569 passed / 0 failed (1562 baseline + 7)
  • cargo test --locked --test corpus381 (380 + 1)
  • make lint — both clippy passes, including the second one naming the
    test = false targets where the new corpus test lives; cargo fmt --check and make check-mod-files clean
  • make assurance — 86/86, 276/276, no dead links
  • Verified at 23bdc93 from a clean detached worktree, not the
    working tree

Notes for a reviewer

  • Connection::changes is still absent. This is the engine half. A Vm
    lives for one statement, so it cannot own sqlite3_changes()'s
    cross-statement retention rule — that is the connection's, and belongs to
    spec 013/Req 1's surface. What the facade has left to do is one line: store
    on Some, ignore None.
  • SELECT is asserted via Program::counts_changes, not through the
    entry point.
    compile_statement handles write and DDL statements only; a
    SELECT reaches the engine by a different route (compile_select* +
    execute_with_db) that has no count to clobber in the first place. The
    static discriminator is what a facade will consult, so it is what the test
    pins.
  • P5 on Insert is now meaningful where its doc comment previously
    said conflict-resolution flags were "not modeled". A future OR REPLACE/OR IGNORE implementation must pick bits other than 0x01.
  • Execution (feat: streaming Execution primitive, with run() as its wrapper (#683) #691) should grow a changes() the same way — three lines
    once both are on main. Whichever merges second.
  • Merge after the spec takeover PR. 013/Req-1 exists only there.
  • No CHANGELOG.md entry or version bump: folded separately per chore: fold #663 into 0.18.9, revert premature 0.18.10 bump #671.

Spend: matched the small estimate. The design work was finding that the
handler-level count is plan-dependent, which cost one afternoon of reading
update.rs rather than any implementation effort.

Refs: 013/Req-1, #678, #683

Closes #692

🤖 Generated with Claude Code

… the opcode (#692)

Nothing in `src/vdbe/` reported how many rows an `INSERT`/`UPDATE`/
`DELETE` changed. Spec 013 calls this the one item on its list a
consumer cannot work around: `execute_transaction_step` returns rows and
the autocommit flag, so a caller cannot tell an `UPDATE` that matched
from one that did not, and that is the distinction every
optimistic-concurrency scheme is built on. SQE swaps a table's metadata
pointer with a conditional `UPDATE` and treats zero rows affected as a
lost race; without the count that becomes SELECT-then-UPDATE, sound only
while the consumer guarantees a single writer.

The obvious implementation is wrong, and measurably so. Counting in the
`Insert`/`Delete` handlers reports, per changed row:

  INSERT                        Insert                          -> 1
  DELETE                        Delete                          -> 1
  UPDATE, single-pass           Delete + Insert                 -> 2
  UPDATE, two-pass range-seek   ephemeral Insert + Delete + Insert -> 3

The two-pass plan is #666/#675's range-seek path, which stashes matched
rowids in an ephemeral b-tree using the same `Opcode::Insert`. So one
`UPDATE` reports 2 or 3 depending on which plan the optimizer picked,
and neither is 1. Index maintenance is the same shape: a write next to
a row that is not a row change.

So codegen marks the one mutation that counts, with `OPFLAG_NCHANGE`
(`0x01`) on `P5` — stock SQLite's flag, same bit, same job. `P5` was
unread by both opcodes, so nothing had to move, and no new opcode means
the frozen-set ADRs (0015/0018/0020) stay closed. An `UPDATE` flags its
`Insert` and not the paired `Delete`: one changed row, counted once.

`StepOutcome::changes` is `Option<u64>`, and the two cases are not the
same answer. `Some(0)` is "this was a DML statement and it changed
nothing" — the lost race. `None` is "not that kind of statement", so a
connection tracking `sqlite3_changes()` leaves its stored count alone
after a `SELECT`. `Program::counts_changes()` is the discriminator and
is deliberately *static*: an `UPDATE` whose `WHERE` matches nothing
never executes its flagged `Insert` but must still report `Some(0)`.

`execute_transaction_step` is now a wrapper over
`execute_transaction_step_counted`, per ADR-0040's pattern — one loop,
the old signature expressed in terms of the new one, so they cannot
drift and its ten existing call sites are untouched.

ADR-0042 records all of it, including why `u64` instead of `Option<u64>`
would be a bug rather than a simplification.

Both wrong designs are mutation-checked, not just argued:

- counting unconditionally in the handlers fails
  `update_of_one_row_reports_one_under_both_plans`,
  `index_maintenance_does_not_count` and
  `conditional_update_reports_match`;
- additionally flagging `UPDATE`'s `Delete` fails the same three, and
  fails the oracle diff with 4 against the oracle's 2.

Verified: 1569 unit tests (1562 baseline + 7) and 381 corpus (380 + 1)
pass, clippy/fmt/mod-files clean, assurance 86/86 and 276/276 with no
dead links. `tests/corpus/changes_oracle_test.rs` diffs a
thirteen-statement sequence against the pinned 3.53.4 oracle's own
`changes()`, covering both `UPDATE` plans, a miss, a partial `DELETE`
and a full one.

Not included: `Connection::changes` and the cross-statement retention
rule. A `Vm` lives for one statement so it cannot own that rule; it is
spec 013/Req 1's surface and belongs to the facade ticket, where it is
one line — store on `Some`, ignore `None`.

One note for whoever merges second: `Execution` (#683) should grow a
`changes()` the same way, which is a three-line addition once both are
on `main`. The requirement IDs cited here live in #678, not yet on
`main`.

Refs: 013/Req-1, #692, #678, #683

Co-Authored-By: Claude Opus 5 (1M context) <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.

feat: a rows-changed counter — the one spec 013 item a consumer cannot work around

1 participant