When processing AddColumn and DropColumn operations, the code looks up tables in transaction_tables.current_tables using the table name stored in the operation. However, transaction_tables.current_tables has tables with their final names (after all renames), while operations store the name at the time they were recorded.
In the failing case:
1. DropColumn was recorded with table_name = "stellar_katerina_672"
2. RenameTable changed the name to "captivating_eco_1159"
3. At commit, when processing DropColumn, we looked for "stellar_katerina_672" but the table was now named "captivating_eco_1159"
The fix: Build a mapping from old names → final names by scanning all RenameTable operations first, then use the final name when looking up in transaction_tables.current_tables.
have to leave for train soon, so claude summary:
Changes in runner/env.rs:
1. Renamed RowOperation to TxOperation
2. Fixed the CreateTable variant to store the full Table instead of separate fields
3. Added CreateIndex and DropIndex variants
4. Added corresponding record_* methods to both TransactionTables and ShadowTablesMut
5. Updated apply_snapshot() to handle CreateTable, CreateIndex, and DropIndex operations in order
6. Removed the index sync loop at the end of apply_snapshot() since operations are now tracked properly
Changes in model/mod.rs:
1. Updated Shadow for Create to call record_create_table() before applying the change
2. Updated Shadow for CreateIndex to call record_create_index() before applying the change
3. Updated Shadow for DropIndex to call record_drop_index() before applying the change
The core fix is that CreateTable and CreateIndex operations are now recorded as they happen, so when apply_snapshot() replays operations in order, subsequent operations like AlterTable find the table already created in committed_tables.
The DX is right now pretty terrible:
```
penberg@vonneumann turso % cargo run -- hello.db
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.15s
Running `target/debug/tursodb hello.db`
Turso v0.4.0-pre.18
Enter ".help" for usage hints.
Did you know that Turso supports live materialized views? Type .manual materialized-views to learn more.
This software is in BETA, use caution with production data and ensure you have backups.
turso> PRAGMA journal_mode = 'experimental_mvcc';
× Invalid argument supplied: MVCC is not enabled. Enable it with `--experimental-mvcc` flag in the CLI or by setting the MVCC option in `DatabaseOpts`
turso>
```
To add insult to the injury, many SDKs don't even have a way to enable
MVCC via database options. Therefore, let's remove the flag altogether.
## Background
In the simulator, we do our best to replicate the effects of an
interactive transaction into the simulator's shadow state whenever that
transaction commits.
## Problem
this didn't work:
BEGIN;
ALTER TABLE t ADD COLUMN foo;
DELETE FROM t WHERE bar != 5;
COMMIT;
None of the rows where bar != 5 were deleted because apply_snapshot()
was checking that the rows in the committed table were exactly equal to
the rows that were recorded, but since the recorded deletes contained a
NULL `foo` column, they never matched. This meant that the sim thought
it should still have all the rows that were deleted.
## Fix:
like all the other operations, record add column / drop column too so
that they are applied in sequential order in apply_snapshot()
No explicit test for this - I ran into this in another branch of mine
whose seed doesn't reproduce on main (because I changed the simulator in
that branch).
Reviewed-by: Pedro Muniz (@pedrocarlo)
Closes#4264
this didn't work:
BEGIN;
ALTER TABLE t ADD COLUMN foo;
DELETE FROM t WHERE bar != 5;
COMMIT;
None of the rows where bar != 5 were deleted because apply_snapshot()
was checking that the rows in the committed table were exactly equal
to the rows that were recorded, but since the recorded deletes contained
a NULL `foo` column, they never matched. This meant that the sim thought
it should still have all the rows that were deleted.
Fix:
like all the other operations, record add column / drop column too so
that they are applied in sequential order in apply_snapshot()
No explicit test for this - I ran into this in another branch of mine
whose seed doesn't reproduce on main (because I changed the simulator
in that branch).
After flushing the WAL, the db may attempt an automatic checkpoint. Previously,
a checkpoint failure for any other reason than `Busy` rolled back the transaction;
this is incorrect because the WAL has already been written and synced, so the transaction
is actually committed and durable.
For this reason, do the following decoupling:
- Mark the transaction as committed after `SyncWal`
- Any errors beyond that point are wrapped as `LimboError::CheckpointFailed` which is
handled specially:
* Tx rollback is not attempted in `abort()` - only the WAL locks are cleaned up
and checkpoint state machines are reset
* In the simulator, the results of the query are shadowed into the sim environment
so that the sim correctly assumes that the transaction's effects were in fact
committed.
Bug 1:
apply_snapshot(), which handles committing transaction changes to the
simulator state, had this bug:
let's say a transaction does:
1. ALTER TABLE t ADD COLUMN (e.g., adding 9th column)
2. INSERT INTO t ... (populating all 9 columns)
then apply_snapshot() would:
1. First push the INSERT row (with 9 values) to committed_tables
2. Then detect new_col_count (9) > old_col_count (8) and add NULLs to ALL rows
hence, adding a 10th column to the new row too.
Fix 1:
only add nulls to existing rows that weren't inserted in the same tx after
the ADD COLUMN.
Bug 2:
the exact same thing, but for DROP COLUMN.
Fix 2:
only remove the i'th column from rows that existed prior to the DROP COLUMN
was issued.
When a new table was created within a transaction, we were incorrectly clearing the rows of the new table when committing.
This resulted in false positives where the sim thought there shouldn't be any
rows in the table.
Fix: don't do that.
e.g. in "ReadYourUpdatesBack", we first do an UPDATE and then a
SELECT to verify that those updates were actually made.
Problem is the SELECT assertion was made even if the UPDATE failed.
Fix: change interaction handling so that if a precondition in a
property fails, the entire property is abandoned.
the issue was that the sim was executing queries on
commit, instead of applying their effects. for example,
if transaction T1 did a `DELETE FROM t WHERE TRUE` it would
delete all the rows even if another mvcc transaction T2 had
inserted rows concurrently that should not be visible to
T1.
WriteWriteConflict should rollback in memory tx but not fail sim.
TxError should not rollback tx but not fail sim either.
Neither of the above should cause sim to shadow the query.
Modify `generation/property.rs` to use the Builder
- add additional metadata to `Interaction` to give more context for
shrinking and iterating over interactions that originated from the
same interaction.
- add Iterator like utilities for `InteractionPlan` to facilitate
iterating over interactions that came from the same property:
will track `Interaction` instead of `Interactions` in the Plan, this
change will impossibilitate the serialization of the InteractionPlan with Serde Json.
- make --load just load the previous cli args
This adds support for running the simulator under Miri to detect UB.
There are a few things to note about Miri and its limitations
- It has limited `libc` coverage, so it's not really possible to have
Miri help with `UringIO`/`UringFile` or `UnixIO`/`UnixFile`. That's a
big gap ☹️
- It **can** work for `GenericIO`/`GenericFile`, which only uses `std`
- It can't call external C libraries, so even using `sqlite` is out
(hence adding `--disable-integrity-check` to the simulator for Miri use)
- It runs on nightly, consequently there are a few new lints that don't
exist on turso's pinned version of rustc
Some questions I have about this MR
- I made `GenericFile::{lock_file,unlock_file}` noops so I could use
`GenericIO`. This isn't great, but if/when you update from Rust 1.88.0
to 1.89.0, `std::File::{lock,lock_shared,unlock}` will be stabilized and
available. Should I note that as a TODO or something?
- Previously, the sim runner shelled out to `git` to get stuff like the
current git hash and the repo directory. For Miri, that's out, and so is
`git2`. Unfortunately, `gix` is also out since it has a required
dependency that uses inline assembly, which Miri doesn't like. I wrote a
hacky shim that uses only std to look for `.git` and find the hash that
HEAD is pointing to. It doesn't deal with stuff like packed-refs or the
repo being a secondary one made with `git worktree`. I'm happy to
support that, but wanted to hear from maintainers before doing more
work.
Two UB occurrences I already found:
- `TursoRwLock::read` used `AtomicU64::compare_exchange_weak`, which is
(evidently) [allowed to spuriously fail](https://doc.rust-lang.org/std/s
ync/atomic/struct.AtomicU64.html#method.compare_exchange_weak) in
exchange for perf. Miri forces this behavior, which triggers trivial
read deadlocks even with zero readers/writers. I changed it to
`compare_exchange`, but I'm not an atomics expert.
- Uninitialized read in non-Unix
`core::storage::buffer_pool::arena::alloc`. This is a simple one,
resolved by using `std::alloc::alloc_zeroed` instead of
`std::alloc::alloc`
Moving forward, I'd be interested in potentially getting the tests to
run in Miri, too. `tokio` looks like a good example of a project with
partial coverage that runs it where they can. They have some extra test
config to allow as many as possible to run under Miri, with
appropriately scaled-down parameter values since Miri is super slow
Closes#3720
gix doesn't work here, since while it's pure Rust, it has a
non-configurable dependency on crates using inline assembly, which Miri
does not support. This commit is a bit of a hack, and only works in
non-bare git repos without e.g packed-refs.
