SQLite table rebuild

[jakobt]

SQLite can't ALTER most of a table, so TableDelta rebuilds it — create, copy, drop,
rename — and those four statements ran with no transaction, each one autocommitting.

Rebuilding a table that another table's foreign key references breaks the database. The
DROP fails on its implicit delete, after the replacement has been created, filled and
committed. The orphan _new table survives, CREATE TABLE IF NOT EXISTS no-ops on it at
the next start, and every run after that fails differently until someone drops it by hand.
Foreign keys are on by default in Microsoft.Data.Sqlite and in all three
SqlitePragmaSettings presets, so this is the ordinary case.

Two smaller ones: an AUTOINCREMENT table came out with a lower sqlite_sequence mark
than it went in with and reissued a used id, and a view over the table failed the rename
outright.

The rebuild now runs the way SQLite documents it — enforcement suspended, the whole thing
in one transaction, foreign_key_check before the commit so a rebuild that really would
dangle a reference rolls back rather than committing the orphan.

One behaviour change: a caller's IMigrationLogger that declines to rethrow used to let a
failed rebuild reach COMMIT half-applied. It's still handed the failure; the migration
no longer continues.

[jeremydmiller]

Merged into 9.29.0 — thanks, this was a good catch and the reproduction was thorough. I pushed one commit to your branch before merging (2a22a8a) with two fixes and two tests. Flagging it since it landed under you.

sqlite_sequence was named unqualified

writeAutoIncrementCarryOver referred to sqlite_sequence bare. It is per-database, and an unqualified name resolves against temp first — so on a connection holding any temp AUTOINCREMENT table, all five references in those two statements went to temp.sqlite_sequence, matched nothing, and carried nothing over. The rebuilt table then reissued an id it had already handed out, which is the failure the carry-over exists to prevent.

Measured on 3.51.0, running your exact SQL. Mark at 5, rows above 2 deleted:

• control: next insert takes id 6 ✅
• with CREATE TEMP TABLE scratch(id INTEGER PRIMARY KEY AUTOINCREMENT, ...) first: next insert takes id 3 — already used by a deleted row

Interesting bit is that #545, your own sibling PR, has a qualify helper whose comment describes this exact resolution order. The two PRs disagreed with each other.

This also needed the temp table to carry the schema of the table being rebuilt. new SqliteObjectName(name + "_new") defaults to main, so a temp-schema rebuild was building main."x_new", copying out of temp."x", dropping it, and leaving the replacement in main. Byte-identical DDL for a main-schema table, so nothing moved for the common case.

foreign_key_check ran unconditionally

pragma_foreign_key_check reports every violation in the table, not only the ones a rebuild could have caused — and a database deliberately running with foreign_keys OFF is allowed to hold dangling rows. Verified: with enforcement off, child(parent_id REFERENCES parent(id)) holding a row pointing at a nonexistent parent makes pragma_foreign_key_check('child','main') return a hit. So rebuilding parent threw and blamed the rebuild for rows it never touched.

Now gated on the setting captured at line 83, which is step 10 of SQLite's own 12-step procedure ("if foreign key constraints were originally enabled").

Tests

Two regression tests in rebuild_is_atomic.cs. I checked they fail first the honest way — reverting only the two fixed conditions and leaving the tests in place turns exactly those two red, with the other 22 still green.

One I did not fix

restorePragmasAsync infers "a transaction is still open" from foreign_keys failing to take the restored value. That only works when the original value was ON — when it was OFF you set the pragma to the value it already holds, so the read-back matches whether or not a transaction is open, and the guard passes in exactly the case its own error message says is dangerous. Left alone to keep the diff reviewable; worth a follow-up if you want it.