diff --git a/.openspec/specs/010-vdbe-write-opcodes/spec.md b/.openspec/specs/010-vdbe-write-opcodes/spec.md index 866e229..df79c43 100644 --- a/.openspec/specs/010-vdbe-write-opcodes/spec.md +++ b/.openspec/specs/010-vdbe-write-opcodes/spec.md @@ -381,13 +381,16 @@ Two ways to satisfy this, and either is acceptable: read-only until it can. Narrower, and it converts silent corruption into an error a caller can act on. -Creating `sqlite_autoindex_*` for a declared constraint is a separate `CREATE +Creating `sqlite_autoindex_*` for a declared constraint was a separate `CREATE TABLE`-side gap (`src/codegen/stmt/insert.rs` records it, V3/V7 owns it), and it -has its own file-level consequence: a table this crate creates with a declared -composite `PRIMARY KEY` has no autoindex, and stock `sqlite3` then answers any +had its own file-level consequence: a table this crate created with a declared +composite `PRIMARY KEY` had no autoindex, and stock `sqlite3` then answered any write or `integrity_check` on it with "database disk image is malformed (11)". -Closing this requirement without closing that one leaves creation broken; closing -that one without this leaves adoption of a foreign file broken. +Closing this requirement without closing that one left creation broken; closing +that one without this left adoption of a foreign file broken. Closed by #687: +`Opcode::CreateTable` now allocates a root page and a `sql IS NULL` +`sqlite_master` row per constraint `schema::autoindex_key_lists` says stock +SQLite would autoindex, numbered by the same rule #685's reader consumes. **Implementation:** `src/schema/ddl_reader.rs::index_schema` (planned), consumed by `src/codegen/stmt/insert.rs::emit_unique_check` @@ -422,6 +425,26 @@ by `src/codegen/stmt/insert.rs::emit_unique_check` **Tests:** `tests/corpus/autoindex_maintenance_test.rs::named_index_round_trips` (planned) +#### Scenario: CREATE TABLE with a declared composite key emits its own autoindex + +- GIVEN `CREATE TABLE t (a INTEGER, b TEXT, PRIMARY KEY (a, b))` run through this + crate +- WHEN the oracle opens the resulting file +- THEN `PRAGMA integrity_check` reports ok, a `sqlite_autoindex_t_1` row exists + with `sql IS NULL`, and the oracle can insert and then rejects a duplicate key + +**Tests:** `tests/corpus/create_table_autoindex_test.rs::composite_primary_key_passes_oracle_integrity_check_and_accepts_oracle_writes`, `tests/corpus/create_table_autoindex_test.rs::autoindex_master_row_shape_matches_oracle_convention` + +#### Scenario: Rowid-alias and WITHOUT ROWID primary keys emit no autoindex on creation + +- GIVEN `CREATE TABLE t (a INTEGER, b TEXT, PRIMARY KEY (a))` (a rowid alias, + #686) and separately the same DDL with `WITHOUT ROWID` appended +- WHEN this crate creates the table +- THEN no `sqlite_autoindex_*` row is registered for either — the constraint + consumes no autoindex number + +**Tests:** `tests/corpus/create_table_autoindex_test.rs::rowid_alias_primary_key_gains_no_autoindex`, `tests/corpus/create_table_autoindex_test.rs::without_rowid_primary_key_gains_no_autoindex` + ## Related regimes - Tier suite: `tests/tiers/tier2.rs::t2_crud_round_trips_on_rowid_tables` diff --git a/CHANGELOG.md b/CHANGELOG.md index c431d7d..36cb57f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,18 @@ All notable changes to sqlite-rs. Format follows [Keep a Changelog](https://keep column list directly, so a second (or later) column no longer defeats the rowid-alias optimization — only a composite key or a non-INTEGER type does (#686). +- `CREATE TABLE` with a declared composite `PRIMARY KEY`/`UNIQUE` + constraint created no `sqlite_autoindex_*` b-tree or `sqlite_master` + row, so stock `sqlite3` answered any write or `integrity_check` + against a table this crate created with "database disk image is + malformed (11)". `Opcode::CreateTable` now allocates a root page and + a `sql IS NULL` `sqlite_master` row for every constraint + `schema::autoindex_key_lists` says stock SQLite would autoindex, + numbered by the same declaration-order rule #685's reader already + consumes; rowid-alias and `WITHOUT ROWID` primary keys still consume + no number. `btree::MasterEntry::sql` is now `Option` so a + `NULL` `sql` column can be represented at all, rather than an empty + string (#687). ## [0.18.10] - 2026-08-31 diff --git a/src/btree/master.rs b/src/btree/master.rs index 6ff8fdb..2070c05 100644 --- a/src/btree/master.rs +++ b/src/btree/master.rs @@ -77,8 +77,10 @@ pub struct MasterEntry { pub tbl_name: String, /// Root page number of the object's b-tree, or 0 when it has none. pub rootpage: u32, - /// The `CREATE ...` SQL text that defines the object. - pub sql: String, + /// The `CREATE ...` SQL text that defines the object, or `None` for + /// an implicitly created `sqlite_autoindex_*` — stock SQLite stores + /// `NULL` there, not an empty string (#687). + pub sql: Option, } /// Scans the table b-tree rooted at `root_page` and returns the highest @@ -145,7 +147,10 @@ pub fn insert_master_row( Value::Text(entry.name.as_str().into()), Value::Text(entry.tbl_name.as_str().into()), Value::Integer(entry.rootpage as i64), - Value::Text(entry.sql.as_str().into()), + match &entry.sql { + Some(sql) => Value::Text(sql.as_str().into()), + None => Value::Null, + }, ]; let payload = encode_record(&values, header.text_encoding); super::insert_row(pager, header, SQLITE_MASTER_ROOT_PAGE, next_rowid, &payload) @@ -203,7 +208,7 @@ pub fn ensure_sqlite_sequence_table( name: "sqlite_sequence".to_string(), tbl_name: "sqlite_sequence".to_string(), rootpage: root_page, - sql: SQLITE_SEQUENCE_SQL.to_string(), + sql: Some(SQLITE_SEQUENCE_SQL.to_string()), }, )?; bump_schema_cookie(pager)?; @@ -323,7 +328,7 @@ pub fn ensure_sqlite_stat1_table( name: "sqlite_stat1".to_string(), tbl_name: "sqlite_stat1".to_string(), rootpage: root_page, - sql: SQLITE_STAT1_SQL.to_string(), + sql: Some(SQLITE_STAT1_SQL.to_string()), }, )?; bump_schema_cookie(pager)?; @@ -445,7 +450,7 @@ mod tests { name: "t".to_string(), tbl_name: "t".to_string(), rootpage: 2, - sql: "CREATE TABLE t(a INTEGER, b TEXT)".to_string(), + sql: Some("CREATE TABLE t(a INTEGER, b TEXT)".to_string()), }, ) .unwrap(); diff --git a/src/btree/schema.rs b/src/btree/schema.rs index 464b798..183e5b3 100644 --- a/src/btree/schema.rs +++ b/src/btree/schema.rs @@ -131,7 +131,7 @@ mod tests { name: "t".to_string(), tbl_name: "t".to_string(), rootpage: table_root, - sql: "CREATE TABLE t(name)".to_string(), + sql: Some("CREATE TABLE t(name)".to_string()), }; let index_root = create_empty_index_root(&mut pager).unwrap(); diff --git a/src/codegen/stmt/insert.rs b/src/codegen/stmt/insert.rs index 6c8425a..7df60a0 100644 --- a/src/codegen/stmt/insert.rs +++ b/src/codegen/stmt/insert.rs @@ -47,11 +47,12 @@ //! seek+branch primitive (`src/vdbe/cursor.rs`, built on //! `IndexCursor::seek`) and dispatches `ON CONFLICT` the same way //! `emit_pk_conflict` does for the rowid-PK case. A composite -//! `PRIMARY KEY(...)`/`UNIQUE(...)` *table* constraint with no backing -//! `CREATE INDEX`/on-disk index (this codebase doesn't auto-create -//! `sqlite_autoindex_*` entries yet) has no real index to seek against, -//! so it still isn't enforced — that's a `CREATE TABLE`-side gap, not -//! an INSERT-codegen one. +//! `PRIMARY KEY(...)`/`UNIQUE(...)` *table* constraint goes through this +//! same path: `CREATE TABLE` now backs it with a `sqlite_autoindex_*` +//! b-tree and `sqlite_master` row (#687), so by the time INSERT compiles +//! there is a real on-disk index in `schema.indexes` for +//! `emit_unique_check` to seek against — no `CREATE TABLE`-side gap is +//! left for this codegen to work around. //! //! Known simplifications (deferred to follow-up tickets, not chased //! here): diff --git a/src/schema.rs b/src/schema.rs index c3fa5cc..9473b41 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -8,7 +8,7 @@ mod ddl_reader; pub use ddl_reader::{ - column_defs, column_type, read_schema, read_schema_and_views, read_table_and_view_names, - read_views, rowid_alias_from_sql, DdlError, IndexSchema, IndexedColumn, TableSchema, - ViewSchema, + autoindex_key_lists, column_defs, column_names_and_without_rowid, column_type, read_schema, + read_schema_and_views, read_table_and_view_names, read_views, rowid_alias_from_sql, DdlError, + IndexSchema, IndexedColumn, TableSchema, ViewSchema, }; diff --git a/src/schema/ddl_reader.rs b/src/schema/ddl_reader.rs index f2598d5..d21cfc7 100644 --- a/src/schema/ddl_reader.rs +++ b/src/schema/ddl_reader.rs @@ -544,7 +544,7 @@ fn same_key_list(a: &[IndexedColumn], b: &[IndexedColumn]) -> bool { /// /// `None` means some constraint could not be understood. The caller must /// treat that as unresolvable and refuse writes, never as "no index". -fn autoindex_key_lists(sql: &str, without_rowid: bool) -> Option>> { +pub fn autoindex_key_lists(sql: &str, without_rowid: bool) -> Option>> { let (start, end) = column_list_span(sql)?; let inner = sql.get(start..end)?; let alias = rowid_alias_column_name(sql, without_rowid); @@ -756,6 +756,20 @@ struct ParsedCreateTable { strict: bool, } +/// Returns a fresh `CREATE TABLE`'s column names (declared order) and +/// whether it is `WITHOUT ROWID` — the two pieces `Opcode::CreateTable` +/// needs to resolve [`autoindex_key_lists`]' `IndexedColumn` names into +/// column positions for [`crate::btree::populate_index_from_table`] +/// (#687). Empty columns and `without_rowid: false` for anything this +/// naive reader can't parse, same degradation as the rest of this +/// module. +pub fn column_names_and_without_rowid(sql: &str) -> (Vec, bool) { + match parse_create_table(sql) { + Some(parsed) => (parsed.columns, parsed.without_rowid), + None => (Vec::new(), false), + } +} + /// Parses `CREATE TABLE ... (col-defs) [table-options]`. Returns `None` /// for anything this naive reader can't find a column list in — the /// caller treats that identically to a virtual table (empty columns, diff --git a/src/vdbe/cursor.rs b/src/vdbe/cursor.rs index beeb123..4409773 100644 --- a/src/vdbe/cursor.rs +++ b/src/vdbe/cursor.rs @@ -2178,15 +2178,16 @@ pub fn create_table(vm: &mut Vm, instr: &Instruction) -> Result &btree::MasterEntry { kind: "table".to_string(), name: name.clone(), - tbl_name: name, + tbl_name: name.clone(), rootpage: root_page, - sql, + sql: Some(sql.clone()), }, ) .map_err(|e| ExecError::MalformedInstruction { opcode: "CreateTable", reason: e.to_string(), })?; + create_declared_autoindexes(&mut pager, &header, &name, &sql, root_page)?; btree::bump_schema_cookie(&mut pager).map_err(|e| ExecError::MalformedInstruction { opcode: "CreateTable", reason: e.to_string(), @@ -2194,6 +2195,70 @@ pub fn create_table(vm: &mut Vm, instr: &Instruction) -> Result Ok(Step::Next) } +/// Emits a `sqlite_autoindex__` entry for each constraint +/// SQLite would autoindex, per the numbering rule #685 derived from the +/// oracle and implements in `schema::autoindex_key_lists` — declaration +/// order, column-level constraints included, rowid-alias/`WITHOUT +/// ROWID` primary keys skipped and consuming no number, redundant +/// constraints collapsed (#687). Runs inside `create_table`'s own write, +/// so a failed `CreateTable` never leaves a partial autoindex behind. +fn create_declared_autoindexes( + pager: &mut crate::pager::Pager, + header: &crate::header::DatabaseHeader, + table_name: &str, + sql: &str, + table_root_page: u32, +) -> Result<(), ExecError> { + let (columns, without_rowid) = crate::schema::column_names_and_without_rowid(sql); + let Some(key_lists) = crate::schema::autoindex_key_lists(sql, without_rowid) else { + return Ok(()); + }; + for (i, key_cols) in key_lists.into_iter().enumerate() { + let ordinal = i.saturating_add(1); + let index_name = format!("sqlite_autoindex_{table_name}_{ordinal}"); + let column_indices: Vec = key_cols + .iter() + .filter_map(|c| { + columns + .iter() + .position(|col| col.eq_ignore_ascii_case(&c.name)) + }) + .collect(); + let index_root = + btree::create_empty_index_root(pager).map_err(|e| ExecError::MalformedInstruction { + opcode: "CreateTable", + reason: e.to_string(), + })?; + btree::populate_index_from_table( + pager, + header, + table_root_page, + index_root, + &column_indices, + ) + .map_err(|e| ExecError::MalformedInstruction { + opcode: "CreateTable", + reason: e.to_string(), + })?; + btree::insert_master_row( + pager, + header, + &btree::MasterEntry { + kind: "index".to_string(), + name: index_name, + tbl_name: table_name.to_string(), + rootpage: index_root, + sql: None, + }, + ) + .map_err(|e| ExecError::MalformedInstruction { + opcode: "CreateTable", + reason: e.to_string(), + })?; + } + Ok(()) +} + /// `CreateView` (#380): registers a `sqlite_master` row with /// `type = 'view'` and `rootpage = 0` — a view has no b-tree of its own, /// so unlike [`create_table`] this never allocates a root page. @@ -2219,7 +2284,7 @@ pub fn create_view(vm: &mut Vm, instr: &Instruction) -> Result name: name.clone(), tbl_name: name, rootpage: 0, - sql, + sql: Some(sql), }, ) .map_err(|e| ExecError::MalformedInstruction { @@ -2343,7 +2408,7 @@ pub fn create_index(vm: &mut Vm, instr: &Instruction) -> Result name: name.clone(), tbl_name: table_name, rootpage: index_root, - sql, + sql: Some(sql), }, ) .map_err(|e| ExecError::MalformedInstruction { diff --git a/tests/corpus/create_table_autoindex_test.rs b/tests/corpus/create_table_autoindex_test.rs new file mode 100644 index 0000000..fff4746 --- /dev/null +++ b/tests/corpus/create_table_autoindex_test.rs @@ -0,0 +1,226 @@ +// Copyright 2026 Schuberg Philis +// SPDX-License-Identifier: Apache-2.0 +//! #687 acceptance: `CREATE TABLE` run through this crate must emit a +//! `sqlite_autoindex_*` for every constraint stock SQLite would +//! autoindex — a declared composite `PRIMARY KEY`/`UNIQUE`, or a +//! non-alias single-column table-level `PRIMARY KEY`. Before this fix, +//! no index b-tree or `sqlite_master` row was created at all, so the +//! oracle reported "database disk image is malformed (11)" on any write +//! or `integrity_check` against a table this crate created with a +//! composite key. +//! +//! This is the producing half of #685 (which fixed adopting an +//! oracle-created autoindex); the numbering rule is shared and already +//! tested there. + +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; +use std::sync::atomic::{AtomicU64, Ordering}; + +use crate::oracle::{assert_integrity_check_ok, pinned_oracle, skip_no_oracle}; + +const CLI: &str = env!("CARGO_BIN_EXE_sqlite-rs"); + +fn scratch_db(label: &str) -> PathBuf { + static COUNTER: AtomicU64 = AtomicU64::new(0); + let n = COUNTER.fetch_add(1, Ordering::Relaxed); + let dir = std::env::temp_dir().join(format!( + "sqlite-rs-create-autoindex-{label}-{}-{n}", + std::process::id() + )); + std::fs::remove_dir_all(&dir).ok(); + std::fs::create_dir_all(&dir).unwrap(); + dir.join("scratch.db") +} + +fn run_ours(db: &Path, sql: &str) -> Output { + Command::new(CLI) + .arg("query") + .arg(db) + .arg(sql) + .output() + .unwrap_or_else(|e| panic!("running {CLI} query {} {sql:?}: {e}", db.display())) +} + +fn create_via_ours(db: &Path, ddl: &str) { + let output = Command::new(CLI) + .arg("exec") + .arg(db) + .arg(ddl) + .output() + .unwrap_or_else(|e| panic!("running {CLI} exec {} {ddl:?}: {e}", db.display())); + assert!( + output.status.success(), + "CREATE TABLE {ddl:?} failed: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +fn oracle_query(oracle: &Path, db: &Path, sql: &str) -> String { + let output = Command::new(oracle) + .arg(db) + .arg(sql) + .output() + .unwrap_or_else(|e| panic!("running oracle on {}: {e}", db.display())); + assert!( + output.status.success(), + "oracle query {sql:?} failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + String::from_utf8_lossy(&output.stdout).into_owned() +} + +fn oracle_exec_ok(oracle: &Path, db: &Path, sql: &str) { + let output = Command::new(oracle) + .arg(db) + .arg(sql) + .output() + .unwrap_or_else(|e| panic!("running oracle exec on {}: {e}", db.display())); + assert!( + output.status.success(), + "oracle exec {sql:?} failed: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +fn oracle_exec_fails(oracle: &Path, db: &Path, sql: &str) { + let output = Command::new(oracle) + .arg(db) + .arg(sql) + .output() + .unwrap_or_else(|e| panic!("running oracle exec on {}: {e}", db.display())); + assert!( + !output.status.success(), + "expected oracle exec {sql:?} to fail, but it succeeded" + ); +} + +/// A table created here with a composite `PRIMARY KEY` must pass the +/// oracle's `PRAGMA integrity_check` and accept an oracle write — +/// the issue's headline "database disk image is malformed (11)" symptom. +#[test] +fn composite_primary_key_passes_oracle_integrity_check_and_accepts_oracle_writes() { + let Some(oracle) = pinned_oracle() else { + skip_no_oracle("create_table_autoindex"); + return; + }; + let db = scratch_db("composite-pk"); + create_via_ours( + &db, + "CREATE TABLE t (a INTEGER, b TEXT, PRIMARY KEY (a, b))", + ); + assert_integrity_check_ok(&oracle, &db); + oracle_exec_ok(&oracle, &db, "INSERT INTO t VALUES (1, 'x')"); + assert_integrity_check_ok(&oracle, &db); + // The autoindex must actually enforce uniqueness for the oracle too. + oracle_exec_fails(&oracle, &db, "INSERT INTO t VALUES (1, 'x')"); +} + +/// A declared composite `UNIQUE` constraint gets the same treatment as a +/// composite `PRIMARY KEY`. +#[test] +fn composite_unique_passes_oracle_integrity_check_and_accepts_oracle_writes() { + let Some(oracle) = pinned_oracle() else { + skip_no_oracle("create_table_autoindex"); + return; + }; + let db = scratch_db("composite-unique"); + create_via_ours(&db, "CREATE TABLE t (a INTEGER, b TEXT, UNIQUE (a, b))"); + assert_integrity_check_ok(&oracle, &db); + oracle_exec_ok(&oracle, &db, "INSERT INTO t VALUES (1, 'x')"); + assert_integrity_check_ok(&oracle, &db); + oracle_exec_fails(&oracle, &db, "INSERT INTO t VALUES (1, 'x')"); +} + +/// `sqlite_master`'s autoindex row must match the oracle's own +/// conventions: `type = 'index'`, the `sqlite_autoindex_
_` +/// name, `tbl_name` the owning table, and — the detail the naive old +/// `MasterEntry` couldn't represent — `sql IS NULL`, not an empty string. +#[test] +fn autoindex_master_row_shape_matches_oracle_convention() { + let Some(oracle) = pinned_oracle() else { + skip_no_oracle("create_table_autoindex"); + return; + }; + let db = scratch_db("master-row-shape"); + create_via_ours( + &db, + "CREATE TABLE t (a INTEGER, b TEXT, PRIMARY KEY (a, b))", + ); + let rows = oracle_query( + &oracle, + &db, + "SELECT type, name, tbl_name, sql IS NULL FROM sqlite_master WHERE type = 'index'", + ); + assert_eq!(rows.trim(), "index|sqlite_autoindex_t_1|t|1"); +} + +/// A non-`WITHOUT ROWID` single-column table-level `PRIMARY KEY` on an +/// `INTEGER` column is a rowid alias (#686) and must get no autoindex at +/// all — the numbering rule's "consumes no number" clause. +#[test] +fn rowid_alias_primary_key_gains_no_autoindex() { + let Some(oracle) = pinned_oracle() else { + skip_no_oracle("create_table_autoindex"); + return; + }; + let db = scratch_db("rowid-alias-no-index"); + create_via_ours(&db, "CREATE TABLE t (a INTEGER, b TEXT, PRIMARY KEY (a))"); + assert_integrity_check_ok(&oracle, &db); + let count = oracle_query( + &oracle, + &db, + "SELECT count(*) FROM sqlite_master WHERE type = 'index' AND tbl_name = 't'", + ); + assert_eq!(count.trim(), "0"); +} + +/// `WITHOUT ROWID`'s own primary key is the table itself and must gain +/// no separate autoindex either. +/// +/// Deliberately does not assert `integrity_check` here: this crate +/// still stores a `WITHOUT ROWID` table's own b-tree as an ordinary +/// rowid table b-tree rather than the index b-tree the file format +/// requires (a pre-existing gap, independent of autoindexing — full +/// `WITHOUT ROWID` write support is unimplemented, not this ticket's +/// scope). Only the autoindex-count claim in #687's scope is checked. +#[test] +fn without_rowid_primary_key_gains_no_autoindex() { + let Some(oracle) = pinned_oracle() else { + skip_no_oracle("create_table_autoindex"); + return; + }; + let db = scratch_db("without-rowid-no-index"); + create_via_ours( + &db, + "CREATE TABLE t (a INTEGER, b TEXT, PRIMARY KEY (a)) WITHOUT ROWID", + ); + let count = oracle_query( + &oracle, + &db, + "SELECT count(*) FROM sqlite_master WHERE type = 'index' AND tbl_name = 't'", + ); + assert_eq!(count.trim(), "0"); +} + +/// Full round trip: create here, write with the oracle, read back here. +#[test] +fn round_trips_create_here_write_oracle_read_here() { + let Some(oracle) = pinned_oracle() else { + skip_no_oracle("create_table_autoindex"); + return; + }; + let db = scratch_db("round-trip"); + create_via_ours( + &db, + "CREATE TABLE t (a INTEGER, b TEXT, PRIMARY KEY (a, b))", + ); + oracle_exec_ok(&oracle, &db, "INSERT INTO t VALUES (1, 'x')"); + let output = run_ours(&db, "SELECT a, b FROM t"); + assert!( + output.status.success(), + "read-back failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!(String::from_utf8_lossy(&output.stdout).trim(), "1|x"); +} diff --git a/tests/corpus/main.rs b/tests/corpus/main.rs index 383fd1c..c904947 100644 --- a/tests/corpus/main.rs +++ b/tests/corpus/main.rs @@ -26,6 +26,7 @@ mod btree_test; mod cli_e2e_test; mod cli_write_test; mod crash_torture_test; +mod create_table_autoindex_test; mod cte_test; mod declared_collate_test; mod dump_oracle_test;