fix(sqlite): delete-all cleared the wrong schema's table - #545
Merged
jeremydmiller merged 1 commit intoSep 2, 2026
Merged
Conversation
GenerateDeleteAllSql built every statement from the table name alone and threw the schema away, so each one went out unqualified and SQLite resolved it by search order -- temp first, then main, then attached databases. Clearing main.records while a temp.records existed emptied temp and left main untouched, and a table in an attached database was addressed wrongly every time, with no error either way. The sqlite_sequence reset had the defect twice over: it hit whichever sqlite_sequence the same search found, and it matched AUTOINCREMENT marks by bare table name, so clearing aux.records reseeded the identity counter of main.records -- silently corrupting identity allocation in a database the caller never named. It now resets each schema's own sqlite_sequence with only that schema's tables. Both statements now name their schema explicitly, including main. The rest of Weasel.Sqlite leaves main unqualified, because DDL creates where it is pointed and the shorter form reads better. This method's whole job is targeting the right table for a destructive statement, and there the unqualified form is exactly the shape a temp table intercepts -- so main, the schema nearly every caller uses, would otherwise keep the bug. Behaviour change worth noting on upgrade: clearing a schema that holds no AUTOINCREMENT table used to emit an unqualified DELETE that found some other schema's sqlite_sequence -- wrong, but quiet. It now emits <schema>.sqlite_sequence and throws "no such table" instead. A loud failure beats a silent wrong answer, but it is a change. Weasel has never guarded this; that guard is its own fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FXHTRGQTNfn7rb9eJPfq4z
Merged
jeremydmiller
added a commit
that referenced
this pull request
Sep 2, 2026
Clears 9.28's known upgrade break, makes the SQLite table rebuild safe against a database that has foreign keys, and adds two capabilities. - #542 closes #538. AssertPatchingIsValid no longer refuses a delta that can rebuild in place, which is what made a numeric or decimal SQLite column throw on 9.28 under every AutoCreate except All. CreateOrUpdate permits a rebuild; CreateOnly still refuses. - #539. The SQLite rebuild runs the way SQLite documents it -- enforcement suspended, one transaction, foreign_key_check before the commit -- instead of four autocommitting statements that wedged the database when the table was referenced. Also fixes an AUTOINCREMENT table reissuing a used id, and a view breaking the rename. - #540. Mutually referencing tables can be created from scratch. A migration holds back only the keys whose target it creates later, so a schema that never had the problem generates byte-for-byte identical DDL. - #543 closes #541. FullTextIndexDefinition can index a pre-built tsvector, making setweight() weighting reachable for JasperFx/marten#5298. - #545. SQLite delete-all names the schema instead of letting SQLite resolve by search order, which put temp first. Minor rather than patch: #543 and #540 add API, and #539 and #542 change what an existing migration does. Ships with one known issue, #546: on SQLite, resetting identity against a schema that has no AUTOINCREMENT table fails with "no such table: <schema>.sqlite_sequence". The single-schema form of this predates 9.29. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 2, 2026
Merged
Member
|
Merged into 9.29.0 unchanged — the schema qualification is right, and the One consequence worth recording, which is why I opened #546 rather than holding this PR. Naming the schema removes the accidental fallback to another schema's copy, so a reset against a schema with no
Two ages of the same bug, and I want to be clear this PR only introduces the second:
I couldn't fix it inside this PR. Three dead ends, all measured on 3.51.0:
#546 has three candidate designs; a connection-aware virtual overload defaulting to the existing sync method looks right to me. Documented as a known issue in the 9.29 guide. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GenerateDeleteAllSql built every statement from the table name alone and threw the schema away, so each one went out unqualified and SQLite resolved it by search order