diff --git a/.openspec/adr/0042-rows-changed-counted-by-codegen-flag.md b/.openspec/adr/0042-rows-changed-counted-by-codegen-flag.md new file mode 100644 index 0000000..f9c7dcc --- /dev/null +++ b/.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`, 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`. diff --git a/.openspec/adr/index.md b/.openspec/adr/index.md index 2b4d7e6..09d3898 100644 --- a/.openspec/adr/index.md +++ b/.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 | diff --git a/Cargo.toml b/Cargo.toml index 40e704c..be40477 100644 --- a/Cargo.toml +++ b/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" diff --git a/src/codegen/stmt/delete.rs b/src/codegen/stmt/delete.rs index bb53f42..46488b4 100644 --- a/src/codegen/stmt/delete.rs +++ b/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)); diff --git a/src/codegen/stmt/insert.rs b/src/codegen/stmt/insert.rs index a9f239a..139811c 100644 --- a/src/codegen/stmt/insert.rs +++ b/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() { diff --git a/src/codegen/stmt/update.rs b/src/codegen/stmt/update.rs index 6785f02..43ecff6 100644 --- a/src/codegen/stmt/update.rs +++ b/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() { diff --git a/src/vdbe.rs b/src/vdbe.rs index 39e3dad..954b7d7 100644 --- a/src/vdbe.rs +++ b/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}; diff --git a/src/vdbe/cursor.rs b/src/vdbe/cursor.rs index d66e819..e1688e2 100644 --- a/src/vdbe/cursor.rs +++ b/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 { 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 { 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) } } diff --git a/src/vdbe/exec.rs b/src/vdbe/exec.rs index 8cef6e7..bc1bb22 100644 --- a/src/vdbe/exec.rs +++ b/src/vdbe/exec.rs @@ -320,6 +320,14 @@ pub struct Vm { /// `Halt` handling for the "BEGIN with no matching COMMIT/ROLLBACK" /// safety fallback. pub(crate) autocommit: bool, + /// Rows this program has changed (013/Req 1, #692) — incremented by + /// `Insert`/`Delete` only when their `P5` carries + /// [`OPFLAG_NCHANGE`], which is codegen's decision rather than the + /// opcode's. Counts table rows: index maintenance (`IdxInsert`, + /// `IdxDelete`, `AutoIndexInsert`) and ephemeral-cursor writes never + /// set the flag, so a table with three indexes reports the same + /// number as the same table with none. + changes: u64, /// Reused byte buffer for `MakeRecord` (#454): amortizes the record /// payload's allocation across every row a statement emits, instead /// of a fresh `Vec` per `MakeRecord` execution. @@ -347,6 +355,7 @@ impl Default for Vm { once_fired: HashSet::new(), params: Vec::new(), autocommit: true, + changes: 0, record_scratch: Vec::new(), make_record_values_scratch: Vec::new(), encode_scratch: Vec::new(), @@ -370,6 +379,27 @@ impl Vm { Self::default() } + /// Counts one changed table row (013/Req 1, #692). Called by + /// `Insert`/`Delete` only when the instruction's `P5` carries + /// [`OPFLAG_NCHANGE`] — see [`Vm::changes`]'s field doc for why the + /// handler cannot decide this for itself. + pub(crate) fn record_change(&mut self) { + self.changes = self.changes.saturating_add(1); + } + + /// Rows changed so far by this program (013/Req 1, #692). + /// + /// This is a per-`Vm` count, and a `Vm` lives for one statement. The + /// cross-statement retention `sqlite3_changes()` specifies — a + /// statement that changes nothing does not clobber the previous + /// count — is the connection's rule, not the engine's, and belongs to + /// spec 013/Req 1's `Connection::changes`. What the engine promises + /// is that this number is right for the statement just run; see + /// [`StepOutcome::changes`] for how the two fit together. + pub fn changes(&self) -> u64 { + self.changes + } + /// Reused scratch buffer for `MakeRecord` (#454) — see /// [`Vm::record_scratch`]'s field doc. pub(crate) fn record_scratch(&mut self) -> &mut Vec { @@ -1009,7 +1039,7 @@ const MAX_STEPS: u32 = 50_000_000; /// Runs `program` to completion on a fresh, database-less [`Vm`] and /// returns the rows it emitted via `ResultRow`. pub fn execute(program: &Program) -> Result>, ExecError> { - run(Vm::new(), program).map(|(rows, _)| rows) + run(Vm::new(), program).map(|(rows, _, _)| rows) } /// Like [`execute`], but binds `params` for `Opcode::Variable` to read @@ -1022,7 +1052,7 @@ pub fn execute_with_params( ) -> Result>, ExecError> { let mut vm = Vm::new(); vm.bind_params(params); - run(vm, program).map(|(rows, _)| rows) + run(vm, program).map(|(rows, _, _)| rows) } /// Like [`execute`], but the `Vm` can service `OpenRead` (cursor @@ -1033,7 +1063,7 @@ pub fn execute_with_db( source: Rc, header: DatabaseHeader, ) -> Result>, ExecError> { - run(Vm::with_db(source, header), program).map(|(rows, _)| rows) + run(Vm::with_db(source, header), program).map(|(rows, _, _)| rows) } /// Like [`execute_with_db`], but the `Vm` can also service the write @@ -1044,7 +1074,7 @@ pub fn execute_with_writable_db( pager: crate::pager::Pager, header: DatabaseHeader, ) -> Result>, ExecError> { - run(Vm::with_writable_db(pager, header), program).map(|(rows, _)| rows) + run(Vm::with_writable_db(pager, header), program).map(|(rows, _, _)| rows) } /// Combines [`execute_with_db`] and [`execute_with_params`]. @@ -1056,7 +1086,7 @@ pub fn execute_with_db_and_params( ) -> Result>, ExecError> { let mut vm = Vm::with_db(source, header); vm.bind_params(params); - run(vm, program).map(|(rows, _)| rows) + run(vm, program).map(|(rows, _, _)| rows) } /// Runs one statement's `program` against a `pager` shared across @@ -1074,12 +1104,58 @@ pub fn execute_transaction_step( header: DatabaseHeader, autocommit_in: bool, ) -> Result<(Vec>, bool), ExecError> { + execute_transaction_step_counted(program, pager, header, autocommit_in) + .map(|outcome| (outcome.rows, outcome.autocommit)) +} + +/// What one statement produced: its rows, the autocommit flag to thread +/// into the next statement, and how many rows it changed (013/Req 1, +/// #692). +pub struct StepOutcome { + /// The statement's result rows, in order. + pub rows: Vec>, + /// Autocommit state after this statement — pass it as the next + /// call's `autocommit_in`, exactly as [`execute_transaction_step`]'s + /// second tuple element. + pub autocommit: bool, + /// Rows changed, or `None` when this statement does not have a + /// rows-changed count at all. + /// + /// `Some(0)` and `None` are different answers and the difference is + /// the whole point. `Some(0)` is "this was an `INSERT`/`UPDATE`/ + /// `DELETE` and it changed nothing" — a lost optimistic-concurrency + /// race, which is what a consumer needs to detect. `None` is "this + /// was not that kind of statement", so a connection tracking + /// `sqlite3_changes()` should leave its stored count untouched rather + /// than zeroing it. That retention rule is the connection's to + /// implement (spec 013/Req 1's `Connection::changes`); this type just + /// makes it a one-liner. + pub changes: Option, +} + +/// [`execute_transaction_step`] plus the rows-changed count (013/Req 1, +/// #692). +/// +/// The two share one loop rather than running in parallel: +/// `execute_transaction_step` is a wrapper that drops the count, so the +/// counted and uncounted paths cannot drift. +pub fn execute_transaction_step_counted( + program: &Program, + pager: Rc>, + header: DatabaseHeader, + autocommit_in: bool, +) -> Result { let mut vm = Vm::with_shared_writable_db(pager, header); vm.autocommit = autocommit_in; - run(vm, program) + let (rows, autocommit, changed) = run(vm, program)?; + Ok(StepOutcome { + rows, + autocommit, + changes: program.counts_changes().then_some(changed), + }) } -fn run(mut vm: Vm, program: &Program) -> Result<(Vec>, bool), ExecError> { +fn run(mut vm: Vm, program: &Program) -> Result<(Vec>, bool, u64), ExecError> { // #509: `steps`/`pc` are both backstops against pathological programs // (a step-limit runaway, a jump target past the end), not values any // real program comes close to overflowing (`MAX_STEPS` is 50_000_000, @@ -1130,7 +1206,7 @@ fn run(mut vm: Vm, program: &Program) -> Result<(Vec>, bool), ExecErr } } } - return Ok((vm.rows, vm.autocommit)); + return Ok((vm.rows, vm.autocommit, vm.changes)); } Step::Halt { code, message } => return Err(ExecError::Halted { code, message }), } diff --git a/src/vdbe/program.rs b/src/vdbe/program.rs index 0763430..7ecf752 100644 --- a/src/vdbe/program.rs +++ b/src/vdbe/program.rs @@ -835,6 +835,19 @@ pub struct Instruction { pub p5: u16, } +/// `P5` bit marking an `Insert`/`Delete` as *the* row change a statement +/// should report through the rows-changed counter (013/Req 1, #692). +/// +/// Same value and same job as stock SQLite's `OPFLAG_NCHANGE`. The flag +/// exists because the opcode alone cannot tell you: one `UPDATE`ed row +/// emits a `Delete` *and* an `Insert` (`codegen/stmt/update.rs`), and the +/// two-pass range-seek plan additionally emits an `Insert` against an +/// ephemeral cursor to stash matched rowids, so counting every +/// `Insert`/`Delete` as it executes reports 3 for a plan that changed 1 +/// row and 2 for the other plan of the same statement. Codegen knows +/// which mutation is the row change; the handler does not. +pub const OPFLAG_NCHANGE: u16 = 0x01; + impl Instruction { /// Builds an instruction with `P4` absent and `P5` zero — the common /// case for control/arithmetic/compare opcodes that only use @@ -861,6 +874,20 @@ impl Instruction { p5: 0, } } + + /// Builds an instruction with an explicit `P5` flags operand — the + /// `with_p4` constructor's counterpart for the flags word. Used for + /// [`OPFLAG_NCHANGE`]; `new`/`with_p4` both leave `p5` zero. + pub fn with_p5(opcode: Opcode, p1: i32, p2: i32, p3: i32, p5: u16) -> Self { + Self { + opcode, + p1, + p2, + p3, + p4: P4::None, + p5, + } + } } /// A linear, zero-indexed sequence of instructions. Execution starts at @@ -878,6 +905,26 @@ impl Program { Self { instructions } } + /// Whether this program is a statement whose rows-changed count is + /// meaningful (013/Req 1, #692) — i.e. whether codegen flagged any + /// mutation with [`OPFLAG_NCHANGE`]. + /// + /// Deliberately *static*: it asks what the program contains, not what + /// it executed. An `UPDATE` whose `WHERE` matches no row never runs + /// its flagged `Insert`, but must still report a count of zero rather + /// than "not a counting statement" — which is the distinction + /// `sqlite3_changes()` needs in order to leave the previous count + /// alone after a `SELECT`. + /// + /// Derived rather than stored so it cannot disagree with the + /// instructions it describes; `Program` is a `Vec` and + /// programs are a handful of instructions per statement. + pub fn counts_changes(&self) -> bool { + self.instructions.iter().any(|i| { + i.p5 & OPFLAG_NCHANGE != 0 && matches!(i.opcode, Opcode::Insert | Opcode::Delete) + }) + } + /// Returns the instruction at `pc`, or `None` if `pc` is out of /// range. pub fn get(&self, pc: usize) -> Option<&Instruction> { diff --git a/tests/corpus/changes_oracle_test.rs b/tests/corpus/changes_oracle_test.rs new file mode 100644 index 0000000..ab42036 --- /dev/null +++ b/tests/corpus/changes_oracle_test.rs @@ -0,0 +1,197 @@ +// Copyright 2026 Schuberg Philis +// SPDX-License-Identifier: Apache-2.0 +//! Oracle diff for the rows-changed counter (spec 013 Requirement 1, +//! #692): runs one statement sequence through this crate's write path and +//! the same sequence through the pinned `sqlite3`, and compares the count +//! each `INSERT`/`UPDATE`/`DELETE` reports. +//! +//! The unit suite (`tests/unit/vdbe_changes_test.rs`) pins the mechanism — +//! that `OPFLAG_NCHANGE` is what counts, so an `UPDATE`'s `Delete` + +//! `Insert` pair is one change and index maintenance is none. This pins +//! the *answers* against the definition of correctness, which is SQLite. + +#![allow( + clippy::unwrap_used, + clippy::expect_used, + clippy::panic, + clippy::indexing_slicing +)] + +use std::cell::RefCell; +use std::path::Path; +use std::process::Command; +use std::rc::Rc; + +use sqlite_rs::btree::TableCursor; +use sqlite_rs::codegen::compile_statement; +use sqlite_rs::header::DatabaseHeader; +use sqlite_rs::pager::Pager; +use sqlite_rs::schema::{read_schema, read_views}; +use sqlite_rs::vdbe::execute_transaction_step_counted; +use sqlite_rs::vfs::MemoryVfs; + +use crate::oracle::{pinned_oracle, skip_no_oracle}; + +/// The sequence both engines run. Every statement is one this crate +/// compiles today, and the `UPDATE`/`DELETE` predicates are chosen to +/// cover a match, a partial match and a miss. +const STATEMENTS: &[&str] = &[ + "CREATE TABLE t(a INTEGER, b TEXT, c TEXT)", + "CREATE INDEX t_a ON t(a)", + "CREATE UNIQUE INDEX t_c ON t(c)", + "INSERT INTO t VALUES (1, 'b1', 'c1')", + "INSERT INTO t VALUES (2, 'b2', 'c2')", + "INSERT INTO t VALUES (3, 'b3', 'c3')", + "INSERT INTO t VALUES (4, 'b4', 'c4')", + // Touches the scanned index -> two-pass plan (#675). + "UPDATE t SET a = a + 10 WHERE a > 2", + // Leaves it alone -> single-pass plan. + "UPDATE t SET b = 'z' WHERE a > 2", + // Matches nothing. + "UPDATE t SET b = 'q' WHERE a = 999", + "DELETE FROM t WHERE a < 3", + "DELETE FROM t", + "DELETE FROM t", +]; + +fn is_dml(sql: &str) -> bool { + let head = sql.trim_start(); + ["INSERT", "UPDATE", "DELETE"] + .iter() + .any(|kw| head.len() >= kw.len() && head[..kw.len()].eq_ignore_ascii_case(kw)) +} + +fn empty_db(page_size: u32) -> (MemoryVfs, DatabaseHeader) { + let mut page1 = vec![0u8; page_size as usize]; + page1[0..16].copy_from_slice(b"SQLite format 3\0"); + page1[16..18].copy_from_slice(&u16::try_from(page_size).unwrap().to_be_bytes()); + page1[18] = 1; + page1[19] = 1; + page1[28..32].copy_from_slice(&1u32.to_be_bytes()); + page1[56..60].copy_from_slice(&1u32.to_be_bytes()); + page1[100] = 0x0D; + page1[105..107].copy_from_slice(&u16::try_from(page_size).unwrap().to_be_bytes()); + + let mut header_bytes = [0u8; 100]; + header_bytes.copy_from_slice(&page1[..100]); + let header = DatabaseHeader::parse(&header_bytes).unwrap(); + + let mut vfs = MemoryVfs::new(); + vfs.insert("/test.db", page1); + (vfs, header) +} + +/// Runs `STATEMENTS` through this crate, returning the count each DML +/// statement reported. +fn ours() -> Vec<(&'static str, Option)> { + let page_size = 4096; + let (vfs, header) = empty_db(page_size); + let pager = Rc::new(RefCell::new( + Pager::open(&vfs, Path::new("/test.db"), page_size).unwrap(), + )); + let mut autocommit = true; + let mut out = Vec::new(); + + for sql in STATEMENTS { + let (schemas, views) = { + let borrowed = pager.borrow(); + let mut schema_cursor = TableCursor::new(&*borrowed, &header, 1); + let schemas = read_schema(&mut schema_cursor, header.text_encoding).unwrap(); + let mut view_cursor = TableCursor::new(&*borrowed, &header, 1); + let views = read_views(&mut view_cursor, header.text_encoding).unwrap(); + (schemas, views) + }; + let program = compile_statement(sql, &schemas, &views) + .unwrap_or_else(|e| panic!("{sql} did not compile: {e}")); + let outcome = + execute_transaction_step_counted(&program, Rc::clone(&pager), header, autocommit) + .unwrap_or_else(|e| panic!("{sql} failed: {e}")); + autocommit = outcome.autocommit; + out.push((*sql, outcome.changes)); + } + out +} + +/// Runs `STATEMENTS` through the pinned oracle, returning `changes()` +/// after each DML statement. +/// +/// One `sqlite3` invocation per statement, with `SELECT changes()` +/// appended: `changes()` is per-connection state, and a fresh invocation +/// starts it at zero, so asking inside the same invocation as the +/// statement is what reports that statement's own count. +fn oracle(bin: &Path, db: &Path) -> Vec<(&'static str, Option)> { + let mut out = Vec::new(); + for sql in STATEMENTS { + let script = if is_dml(sql) { + format!("{sql};\nSELECT changes();") + } else { + format!("{sql};") + }; + let output = Command::new(bin) + .arg(db) + .arg(&script) + .output() + .unwrap_or_else(|e| panic!("oracle failed to run {sql}: {e}")); + assert!( + output.status.success(), + "oracle rejected {sql}: {}", + String::from_utf8_lossy(&output.stderr) + ); + let changed = if is_dml(sql) { + Some( + String::from_utf8_lossy(&output.stdout) + .trim() + .parse::() + .unwrap_or_else(|e| { + panic!( + "oracle's changes() after {sql} was not a number ({e}): {:?}", + String::from_utf8_lossy(&output.stdout) + ) + }), + ) + } else { + None + }; + out.push((*sql, changed)); + } + out +} + +#[test] +fn rows_changed_counts_match_the_oracle() { + let Some(bin) = pinned_oracle() else { + skip_no_oracle("rows_changed_counts_match_the_oracle"); + return; + }; + let dir = tempdir(); + let db = dir.join("changes.db"); + + let mine = ours(); + let theirs = oracle(&bin, &db); + + // Compare only the DML statements: `changes()` is undefined-by-design + // for a DDL statement (the oracle reports whatever the connection's + // previous count was; we report `None`), so the interesting claim is + // the counting statements agreeing exactly. + let mine_dml: Vec<_> = mine.iter().filter(|(sql, _)| is_dml(sql)).collect(); + let theirs_dml: Vec<_> = theirs.iter().filter(|(sql, _)| is_dml(sql)).collect(); + + assert_eq!(mine_dml, theirs_dml, "rows-changed counts diverge"); + + // And the DDL half really is `None` on our side rather than an + // accidental `Some(0)`, which is the distinction a connection needs + // in order not to clobber a retained count. + for (sql, changed) in &mine { + if !is_dml(sql) { + assert_eq!(*changed, None, "{sql} claimed a rows-changed count"); + } + } + + std::fs::remove_dir_all(&dir).ok(); +} + +fn tempdir() -> std::path::PathBuf { + let dir = std::env::temp_dir().join(format!("sqlite-rs-changes-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + dir +} diff --git a/tests/corpus/main.rs b/tests/corpus/main.rs index 96ac787..10dbad2 100644 --- a/tests/corpus/main.rs +++ b/tests/corpus/main.rs @@ -13,6 +13,7 @@ clippy::arithmetic_side_effects )] +mod changes_oracle_test; mod harness; mod oracle; diff --git a/tests/unit/vdbe_changes_test.rs b/tests/unit/vdbe_changes_test.rs new file mode 100644 index 0000000..762b52b --- /dev/null +++ b/tests/unit/vdbe_changes_test.rs @@ -0,0 +1,299 @@ +// Copyright 2026 Schuberg Philis +// SPDX-License-Identifier: Apache-2.0 +//! Rows-changed counter (spec 013 Requirement 1, #692). +//! +//! Spec 013 calls this the one item on its list a consumer cannot work +//! around: without it a caller cannot tell an `UPDATE` that matched from +//! one that did not, which is the distinction every +//! optimistic-concurrency scheme is built on. +//! +//! The counter is driven by `OPFLAG_NCHANGE` on `P5` rather than by the +//! opcode, and these tests are the reason. One `UPDATE`ed row emits a +//! `Delete` *and* an `Insert`, and the two-pass range-seek plan +//! (#666/#675) emits a third `Insert` against an ephemeral cursor to +//! stash matched rowids — so a handler that counted every +//! `Insert`/`Delete` would report 3 for a plan that changed 1 row, and 2 +//! for the other plan of the same statement. +//! `update_of_one_row_reports_one_under_both_plans` pins exactly that. + +#![allow( + clippy::unwrap_used, + clippy::expect_used, + clippy::panic, + clippy::indexing_slicing +)] + +use std::cell::RefCell; +use std::path::Path; +use std::rc::Rc; + +use sqlite_rs::btree::TableCursor; +use sqlite_rs::codegen::{compile_select_with_catalog, compile_statement}; +use sqlite_rs::header::DatabaseHeader; +use sqlite_rs::pager::Pager; +use sqlite_rs::parser::error::ParseOutcome; +use sqlite_rs::parser::parse_select; +use sqlite_rs::record::Value; +use sqlite_rs::schema::{read_schema, read_views, TableSchema, ViewSchema}; +use sqlite_rs::vdbe::{execute_transaction_step_counted, Program, StepOutcome}; +use sqlite_rs::vfs::MemoryVfs; + +fn empty_db(page_size: u32) -> (MemoryVfs, DatabaseHeader) { + let mut page1 = vec![0u8; page_size as usize]; + page1[0..16].copy_from_slice(b"SQLite format 3\0"); + page1[16..18].copy_from_slice(&u16::try_from(page_size).unwrap().to_be_bytes()); + page1[18] = 1; + page1[19] = 1; + page1[28..32].copy_from_slice(&1u32.to_be_bytes()); + page1[56..60].copy_from_slice(&1u32.to_be_bytes()); + page1[100] = 0x0D; + page1[105..107].copy_from_slice(&u16::try_from(page_size).unwrap().to_be_bytes()); + + let mut header_bytes = [0u8; 100]; + header_bytes.copy_from_slice(&page1[..100]); + let header = DatabaseHeader::parse(&header_bytes).unwrap(); + + let mut vfs = MemoryVfs::new(); + vfs.insert("/test.db", page1); + (vfs, header) +} + +struct Db { + pager: Rc>, + header: DatabaseHeader, + autocommit: bool, +} + +impl Db { + fn new() -> Self { + let page_size = 4096; + let (vfs, header) = empty_db(page_size); + let pager = Pager::open(&vfs, Path::new("/test.db"), page_size).unwrap(); + Self { + pager: Rc::new(RefCell::new(pager)), + header, + autocommit: true, + } + } + + fn catalog(&self) -> (Vec, Vec) { + let borrowed = self.pager.borrow(); + let mut schema_cursor = TableCursor::new(&*borrowed, &self.header, 1); + let schemas = read_schema(&mut schema_cursor, self.header.text_encoding).unwrap(); + let mut view_cursor = TableCursor::new(&*borrowed, &self.header, 1); + let views = read_views(&mut view_cursor, self.header.text_encoding).unwrap(); + (schemas, views) + } + + fn step(&mut self, sql: &str) -> StepOutcome { + let (schemas, views) = self.catalog(); + let program = compile_statement(sql, &schemas, &views).unwrap(); + let outcome = execute_transaction_step_counted( + &program, + Rc::clone(&self.pager), + self.header, + self.autocommit, + ) + .unwrap(); + self.autocommit = outcome.autocommit; + outcome + } + + /// Runs a statement that has no rows-changed count (DDL), asserting + /// that it does not claim one. + fn exec_ddl(&mut self, sql: &str) { + assert_eq!( + self.step(sql).changes, + None, + "{sql} should not be a counting statement" + ); + } + + /// The rows-changed count for `sql`, asserting it is a counting + /// statement at all. + fn changes(&mut self, sql: &str) -> u64 { + self.step(sql) + .changes + .unwrap_or_else(|| panic!("{sql} reported no rows-changed count")) + } + + /// Compiles a write/DDL statement without running it. + fn compile(&mut self, sql: &str) -> Program { + let (schemas, views) = self.catalog(); + compile_statement(sql, &schemas, &views).unwrap() + } + + /// Compiles a `SELECT`, which `compile_statement` does not handle. + fn compile_select(&mut self, sql: &str) -> Program { + let (schemas, _) = self.catalog(); + let select = match parse_select(sql) { + ParseOutcome::Accepted(s) => s, + other => panic!("{sql} did not parse: {other:?}"), + }; + let schema = schemas + .iter() + .find(|s| s.name.eq_ignore_ascii_case("t")) + .unwrap(); + compile_select_with_catalog(&select, schema, &schemas).unwrap() + } + + fn count_rows(&mut self, sql: &str) -> i64 { + let program = self.compile_select(sql); + let outcome = execute_transaction_step_counted( + &program, + Rc::clone(&self.pager), + self.header, + self.autocommit, + ) + .unwrap(); + self.autocommit = outcome.autocommit; + match &outcome.rows[0][0] { + Value::Integer(n) => *n, + other => panic!("expected an integer, got {other:?}"), + } + } +} + +/// Spec 013/Req 1's first scenario: the same conditional `UPDATE` run +/// twice reports one row changed, then zero. +#[test] +fn conditional_update_reports_match() { + let mut db = Db::new(); + db.exec_ddl("CREATE TABLE t(k TEXT, metadata_location TEXT)"); + db.changes("INSERT INTO t VALUES ('a', 'a')"); + + let first = db.changes("UPDATE t SET metadata_location = 'b' WHERE metadata_location = 'a'"); + let second = db.changes("UPDATE t SET metadata_location = 'b' WHERE metadata_location = 'a'"); + + assert_eq!(first, 1, "the update that matched"); + assert_eq!(second, 0, "the same update, now losing the race"); +} + +/// The regression guard for `OPFLAG_NCHANGE`'s existence. Both `UPDATE` +/// plans change exactly one row and must both say so, even though they +/// emit different numbers of `Insert`/`Delete` opcodes to do it. +/// +/// Plan selection is #675's rule: the two-pass plan is taken when the +/// `SET` clause touches the index the range predicate scans, the +/// single-pass plan when it does not. +#[test] +fn update_of_one_row_reports_one_under_both_plans() { + // Two-pass: `SET n` touches the scanned index `t_n`. + let mut two_pass = Db::new(); + two_pass.exec_ddl("CREATE TABLE t(n INTEGER, v TEXT)"); + two_pass.exec_ddl("CREATE INDEX t_n ON t(n)"); + for i in 1..=5 { + two_pass.changes(&format!("INSERT INTO t VALUES ({i}, 'v{i}')")); + } + let counted = two_pass.changes("UPDATE t SET n = n + 100 WHERE n > 4"); + + // Single-pass: `SET v` leaves the scanned index alone. + let mut single_pass = Db::new(); + single_pass.exec_ddl("CREATE TABLE t(n INTEGER, v TEXT)"); + single_pass.exec_ddl("CREATE INDEX t_n ON t(n)"); + for i in 1..=5 { + single_pass.changes(&format!("INSERT INTO t VALUES ({i}, 'v{i}')")); + } + let counted_single = single_pass.changes("UPDATE t SET v = 'x' WHERE n > 4"); + + assert_eq!(counted, 1, "two-pass plan counted its own scratch writes"); + assert_eq!(counted_single, 1, "single-pass plan"); +} + +/// `Some(0)` and `None` are different answers: an `UPDATE` that matched +/// nothing is a counting statement that counted zero, which is what a +/// lost optimistic-concurrency race looks like. +#[test] +fn update_matching_nothing_reports_some_zero_not_none() { + let mut db = Db::new(); + db.exec_ddl("CREATE TABLE t(n INTEGER)"); + db.changes("INSERT INTO t VALUES (1)"); + + let outcome = db.step("UPDATE t SET n = 2 WHERE n = 999"); + + assert_eq!( + outcome.changes, + Some(0), + "an UPDATE whose WHERE matches nothing still has a count" + ); +} + +/// A `SELECT` has no rows-changed count, so a connection tracking +/// `sqlite3_changes()` leaves its stored value alone rather than zeroing +/// it (spec 013/Req 1's second scenario). +/// +/// Asserted against `Program::counts_changes` rather than through +/// `execute_transaction_step_counted`, because `compile_statement` +/// handles write and DDL statements only — a `SELECT` reaches the engine +/// by a different route entirely (`compile_select*` + `execute_with_db`), +/// which has no count to clobber in the first place. The static +/// discriminator is the thing a future facade will consult, so it is the +/// thing worth pinning. +#[test] +fn select_is_not_a_counting_statement() { + let mut db = Db::new(); + db.exec_ddl("CREATE TABLE t(n INTEGER)"); + db.changes("INSERT INTO t VALUES (1)"); + + for sql in ["SELECT n FROM t WHERE n = 999", "SELECT count(*) FROM t"] { + let program = db.compile_select(sql); + assert!( + !program.counts_changes(), + "{sql} claimed a rows-changed count" + ); + } + + // And a statement that does have one still says so, so the assertion + // above is not passing for want of any flagged program at all. + let insert = db.compile("INSERT INTO t VALUES (2)"); + assert!(insert.counts_changes()); +} + +#[test] +fn insert_and_delete_count_their_rows() { + let mut db = Db::new(); + db.exec_ddl("CREATE TABLE t(n INTEGER)"); + for i in 0..7 { + assert_eq!(db.changes(&format!("INSERT INTO t VALUES ({i})")), 1); + } + assert_eq!(db.count_rows("SELECT count(*) FROM t"), 7); + + assert_eq!(db.changes("DELETE FROM t WHERE n < 3"), 3, "partial delete"); + assert_eq!(db.changes("DELETE FROM t"), 4, "the rest"); + assert_eq!(db.changes("DELETE FROM t"), 0, "already empty"); +} + +/// Index maintenance is a row-adjacent write, not a row change: the same +/// statements against a table with three indexes must report the same +/// numbers as against a table with none. +#[test] +fn index_maintenance_does_not_count() { + let counts = |indexed: bool| { + let mut db = Db::new(); + db.exec_ddl("CREATE TABLE t(a INTEGER, b TEXT, c TEXT)"); + if indexed { + db.exec_ddl("CREATE INDEX t_a ON t(a)"); + db.exec_ddl("CREATE INDEX t_b ON t(b)"); + db.exec_ddl("CREATE UNIQUE INDEX t_c ON t(c)"); + } + let mut seen = Vec::new(); + for i in 0..4 { + seen.push(db.changes(&format!("INSERT INTO t VALUES ({i}, 'b{i}', 'c{i}')"))); + } + seen.push(db.changes("UPDATE t SET b = 'z' WHERE a >= 2")); + seen.push(db.changes("DELETE FROM t WHERE a < 2")); + seen + }; + + assert_eq!(counts(true), counts(false)); + assert_eq!(counts(false), vec![1, 1, 1, 1, 2, 2]); +} + +/// DDL is not a counting statement either, even though `CREATE TABLE` +/// writes a `sqlite_master` row. +#[test] +fn ddl_has_no_count() { + let mut db = Db::new(); + assert_eq!(db.step("CREATE TABLE t(n INTEGER)").changes, None); + assert_eq!(db.step("CREATE INDEX t_n ON t(n)").changes, None); +}