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

.openspec/adr/0042-rows-changed-counted-by-codegen-flag.md

@@ -0,0 +1,103 @@
+# 0042 — Codegen decides which mutation is a row change, and `None` is not zero
+
+**Status:** Accepted · **Date:** 2026-09-04
+
+## Context
+
+Spec 013 Requirement 1 asks for `sqlite3_changes()`: how many rows the last
+`INSERT`/`UPDATE`/`DELETE` changed. Spec 013 calls it the one item on its list
+a consumer cannot work around, because without it a caller cannot distinguish
+an `UPDATE` that matched from one that did not, and that distinction is what
+every optimistic-concurrency scheme is built on.
+
+The obvious implementation — increment a counter in the `Insert` and `Delete`
+opcode handlers — is wrong, and measurably so on the tree at 0.18.10:
+
+| statement | opcodes emitted per row | counted naively |
+|---|---|---|
+| `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`
+would report 2 or 3 depending on which plan the optimizer picked, and neither
+is 1. Index maintenance (`IdxInsert`, `IdxDelete`, `AutoIndexInsert`) has the
+same character: a write, adjacent to a row, that is not a row change.
+
+The opcode does not carry enough information to answer. Codegen does.
+
+## Decision
+
+**Codegen marks the one mutation that is the row change, with
+`OPFLAG_NCHANGE` (`0x01`) on `P5`.** Same bit and same job as stock SQLite's
+flag of that name. `cursor::insert`/`cursor::delete` increment `Vm`'s counter
+only when it is set; `Instruction::with_p5` is the constructor that sets it,
+alongside the existing `with_p4`. `P5` was unread by both opcodes, so nothing
+had to move.
+
+An `UPDATE` flags its `Insert` and not the paired `Delete` — one changed row,
+counted once. `INSERT` flags its table `Insert`; `DELETE` flags both of its
+`Delete` sites. Nothing else is ever flagged.
+
+**The count is exposed as `Option<u64>`, and `None` is not `Some(0)`.**
+`StepOutcome::changes` is `Some(n)` when the program is a counting statement
+and `None` when it is not:
+
+- `Some(0)` means "this was an `INSERT`/`UPDATE`/`DELETE` and it changed
+  nothing" — a lost optimistic-concurrency race, which is the case the
+  requirement exists to make visible.
+- `None` means "not that kind of statement", so a connection tracking
+  `sqlite3_changes()` leaves its stored count alone.
+
+The discriminator is **static** — `Program::counts_changes()` 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)`.
+
+**`execute_transaction_step` becomes a wrapper** over
+`execute_transaction_step_counted`, which returns the count. Same pattern
+ADR-0040 settled on for streaming: one loop, the older signature expressed in
+terms of the newer one, so the two cannot drift and the existing suite is the
+equivalence proof.
+
+## Alternatives rejected
+
+- **Count in the handlers, unconditionally.** Reports 2 or 3 for a one-row
+  `UPDATE`, plan-dependently, and counts index maintenance. This is the
+  alternative the table above exists to close, and
+  `update_of_one_row_reports_one_under_both_plans` is its regression guard:
+  removing the flag check fails that test and two others.
+- **Return `u64` and let `0` mean both.** Collapses "changed nothing" into
+  "not a counting statement", which is exactly the distinction SQLite's
+  retention rule is built on — a `SELECT` would zero a count that should have
+  survived it. The two-case type is the whole point and should not be
+  simplified away.
+- **A `Program { counts_changes: bool }` field set by codegen.** Equivalent
+  in behaviour, but it can disagree with the instructions it describes, and
+  `Program::new` has many call sites. Deriving it costs one pass over a
+  handful of instructions.
+- **Change `execute_transaction_step`'s return type in place.** Ten call
+  sites across `src/bin/`, tests, benches and examples, for a value almost
+  none of them want. The wrapper is free.
+- **Track the count across statements in the `Vm`.** A `Vm` lives for one
+  statement, so it cannot. Cross-statement retention is the connection's
+  rule and belongs to spec 013/Req 1's `Connection::changes`.
+
+## Consequences
+
+The number is correct for the statement just run, verified against the pinned
+3.53.4 oracle's own `changes()` for a thirteen-statement sequence covering
+both `UPDATE` plans, a miss, a partial `DELETE` and a full one
+(`tests/corpus/changes_oracle_test.rs`). Both wrong designs above were
+mutation-checked against that test as well as the unit suite.
+
+`Connection::changes` is still absent — this is the engine half. What the
+facade has left to do is one line: store the value on `Some`, ignore `None`.
+
+Adding a `P5` flag reopens no frozen set: no new opcode, so ADR-0015, ADR-0018
+and ADR-0020 are untouched. But `P5` on `Insert` is now meaningful where its
+doc comment previously said conflict-resolution flags were "not modeled", so a
+future `OR REPLACE`/`OR IGNORE` implementation must pick bits other than
+`0x01`.

.openspec/adr/index.md

@@ -42,3 +42,4 @@ Specs record what the system must do; ADRs record **why it is shaped this way**
 | [0036](0036-pragma-synchronous-fsync-policy.md) | `PRAGMA synchronous` fsync-skip policy, and why `SynchronousMode` lives in `header.rs` | 2026-08-29 |
 | [0037](0037-macos-plain-fsync-not-fullfsync.md) | On macOS, `Vfs::sync` calls plain `fsync(2)`, not `std`'s `F_FULLFSYNC` | 2026-08-30 |
 | [0038](0038-cargo-registry-opt-in-not-committed.md) | Artifactory Cargo access is opt-in local config, never a committed source replacement | 2026-09-03 |
+| [0042](0042-rows-changed-counted-by-codegen-flag.md) | Codegen flags the one mutation that is a row change; `None` is not `Some(0)` | 2026-09-04 |

Cargo.toml

@@ -128,6 +128,10 @@ path = "tests/unit/vdbe_write_opcodes_test.rs"
 name = "vdbe_integrity_check"
 path = "tests/unit/vdbe_integrity_check_test.rs"
 
+[[test]]
+name = "vdbe_changes"
+path = "tests/unit/vdbe_changes_test.rs"
+
 [[test]]
 name = "codegen_expr"
 path = "tests/unit/codegen_expr_test.rs"

src/codegen/stmt/delete.rs

@@ -28,7 +28,7 @@ use crate::codegen::select::{is_rowid_reference, top_level_equality_operands, Co
 use crate::codegen::{CondTargets, Emitter, RegAlloc, Scope, Target};
 use crate::parser::ast::{Delete, ExprKind, Literal, ParamKind};
 use crate::schema::TableSchema;
-use crate::vdbe::{Instruction, Opcode, Program};
+use crate::vdbe::{Instruction, Opcode, Program, OPFLAG_NCHANGE};
 
 const TABLE_CURSOR: i32 = 0;
 const FIRST_INDEX_CURSOR: i32 = 1;
@@ -118,7 +118,13 @@ pub fn compile_delete_with_catalog(
             FIRST_INDEX_CURSOR,
             Opcode::IdxDelete,
         )?;
-        em.emit(Instruction::new(Opcode::Delete, TABLE_CURSOR, 0, 0));
+        em.emit(Instruction::with_p5(
+            Opcode::Delete,
+            TABLE_CURSOR,
+            0,
+            0,
+            OPFLAG_NCHANGE,
+        ));
 
         em.place(end_label);
         em.emit(Instruction::new(Opcode::Halt, 0, 0, 0));
@@ -149,7 +155,13 @@ pub fn compile_delete_with_catalog(
         FIRST_INDEX_CURSOR,
         Opcode::IdxDelete,
     )?;
-    em.emit(Instruction::new(Opcode::Delete, TABLE_CURSOR, 0, 0));
+    em.emit(Instruction::with_p5(
+        Opcode::Delete,
+        TABLE_CURSOR,
+        0,
+        0,
+        OPFLAG_NCHANGE,
+    ));
 
     em.place(row_skip);
     let next_addr = em.emit(Instruction::new(Opcode::Next, TABLE_CURSOR, 0, 0));

src/codegen/stmt/insert.rs

@@ -95,7 +95,7 @@ use crate::parser::ast::{
 use crate::parser::error::ParseOutcome;
 use crate::parser::parse_create_table;
 use crate::schema::TableSchema;
-use crate::vdbe::{affinity_of, Instruction, Opcode, Program, P4};
+use crate::vdbe::{affinity_of, Instruction, Opcode, Program, OPFLAG_NCHANGE, P4};
 
 /// Process-wide cache of parsed `CREATE TABLE` DDL, keyed by the exact
 /// `schema.sql` text — content-addressed, since the parse result depends
@@ -786,11 +786,15 @@ fn compile_row(
         record_reg,
         P4::Affinity(affinities),
     ));
-    em.emit(Instruction::new(
+    // `OPFLAG_NCHANGE`: this is the one mutation an INSERT reports as a
+    // changed row (013/Req 1, #692). The index maintenance emitted just
+    // below is deliberately unflagged.
+    em.emit(Instruction::with_p5(
         Opcode::Insert,
         TABLE_CURSOR,
         rowid_reg,
         record_reg,
+        OPFLAG_NCHANGE,
     ));
 
     if !schema.indexes.is_empty() {

src/codegen/stmt/update.rs

@@ -55,7 +55,7 @@ use crate::parser::ast::{
     ConflictAction, Expr, ExprKind, Literal, ParamKind, TableConstraint, Update,
 };
 use crate::schema::TableSchema;
-use crate::vdbe::{affinity_of, Instruction, Opcode, Program, P4};
+use crate::vdbe::{affinity_of, Instruction, Opcode, Program, OPFLAG_NCHANGE, P4};
 
 const TABLE_CURSOR: i32 = 0;
 const CHECK_CURSOR: i32 = 1;
@@ -600,12 +600,18 @@ fn emit_update_row_body(
         FIRST_INDEX_CURSOR,
         Opcode::IdxDelete,
     )?;
+    // One changed row, counted once (013/Req 1, #692): an UPDATE rewrites
+    // a row as `Delete` + `Insert`, so only the `Insert` carries
+    // `OPFLAG_NCHANGE`. Flagging both would report 2 per row; flagging the
+    // `Delete` instead would work equally well but reads as a deletion.
+    // Stock SQLite flags the insert side too.
     em.emit(Instruction::new(Opcode::Delete, TABLE_CURSOR, 0, 0));
-    em.emit(Instruction::new(
+    em.emit(Instruction::with_p5(
         Opcode::Insert,
         TABLE_CURSOR,
         rowid_reg,
         record_reg,
+        OPFLAG_NCHANGE,
     ));
 
     if !schema.indexes.is_empty() {

src/vdbe.rs

@@ -35,8 +35,9 @@ pub use control::{
     TRANSACTION_MODE_DEFERRED, TRANSACTION_MODE_EXCLUSIVE, TRANSACTION_MODE_IMMEDIATE,
 };
 pub use exec::{
-    execute, execute_transaction_step, execute_with_db, execute_with_db_and_params,
-    execute_with_params, execute_with_writable_db, ExecError, Step, Vm,
+    execute, execute_transaction_step, execute_transaction_step_counted, execute_with_db,
+    execute_with_db_and_params, execute_with_params, execute_with_writable_db, ExecError, Step,
+    StepOutcome, Vm,
 };
 pub use explain::{explain, ExplainRow};
 pub use functions::{call as call_function, like_match, FunctionError};
@@ -46,6 +47,6 @@ pub use pragma::{
 };
 pub use program::{
     AnalyzeIndexTarget, AnalyzeTarget, GroupKeyColumn, Instruction, Opcode, Program, SortKeyColumn,
-    P4,
+    OPFLAG_NCHANGE, P4,
 };
 pub use value::{and, is, is_not, not, or, sql_eq, sql_lt};

src/vdbe/cursor.rs

@@ -58,7 +58,7 @@ use crate::record::{
     record_column_count, TextEncoding, Value,
 };
 use crate::vdbe::exec::{to_pc, ExecError, Step, Vm};
-use crate::vdbe::program::{Instruction, P4};
+use crate::vdbe::program::{Instruction, OPFLAG_NCHANGE, P4};
 use crate::vdbe::{compare, Collation};
 
 /// One open cursor slot: a real table cursor, an in-memory ephemeral
@@ -1964,6 +1964,13 @@ pub fn delete(vm: &mut Vm, instr: &Instruction) -> Result<Step, ExecError> {
             if let CursorSlot::Table(state) = vm.cursor_mut(instr.p1)? {
                 state.set_current(None);
             }
+            // Only when codegen marked this delete as the statement's row
+            // change (#692). A DELETE's own `Delete` carries the flag; the
+            // one an UPDATE emits before re-inserting the row does not,
+            // because that pair is one changed row, not two.
+            if instr.p5 & OPFLAG_NCHANGE != 0 {
+                vm.record_change();
+            }
             Ok(Step::Next)
         }
         CursorSlot::Ephemeral(_) => {
@@ -2048,6 +2055,14 @@ pub fn insert(vm: &mut Vm, instr: &Instruction) -> Result<Step, ExecError> {
                     reason: e.to_string(),
                 }
             })?;
+            drop(pager);
+            // See `Delete` above: the flag is codegen's call. Note the
+            // `EphemeralTable` arm never reaches here, so the two-pass
+            // UPDATE plan's rowid-stashing `Insert` cannot count even if
+            // a future caller flagged it by mistake.
+            if instr.p5 & OPFLAG_NCHANGE != 0 {
+                vm.record_change();
+            }
             Ok(Step::Next)
         }
     }