Fire sqlite3_update_hook on clustered PRIMARY KEY tables - #2432
Merged
Conversation
|
SummaryCoverage spans normal database writes and reads across integer, text, composite, clustered, and rowid-free tables, including callback behavior, row identity consistency, updates, deletes, and row transfers. It also exercises edge and adversarial conditions such as duplicate or failed writes, no-op statements, concurrent writers, key changes, and compatibility between clustered and ordinary tables. Safe to merge — the exercised database behaviors are healthy across normal, edge-case, failure-recovery, concurrency, and compatibility scenarios, with no PR-attributable regressions or unresolved failures. No merge blocker is indicated by this run. Tests run by Ito
Tip Reply with @itoqa to send us feedback on this test run. |
Contributor
DoltLite performance vs PR base
blobpk details
compositepk details
int details
textpk details
vc details
All relative performance gates passed. |
Non-INTEGER PRIMARY KEY tables are stored as WITHOUT ROWID, so INSERT and UPDATE go through OP_IdxInsert and never reached the OP_Insert update hook. DELETE already used OP_Delete but skipped the hook when HasRowid was false. For VisibleRowid clustered tables, attach the Table to the PK OP_IdxInsert and invoke the hook with the synthetic SQL rowid. DELETE captures sqlite3BtreeSqlRowid before the btree remove. Explicit WITHOUT ROWID stays silent, matching stock. Fixes #2427. Co-Authored-By: Grok 4.6 <noreply@x.ai>
P4_TABLE on the PK IdxInsert stole the unpacked nMem count. Zeroing x.nMem made WITHOUT ROWID CASCADE UPDATE compare the full packed record, miss the existing PK, and insert duplicate child rows (fkey8-7.4). Restore nMem from KeyInfo.nKeyField when P3 names the unpacked key. Co-Authored-By: Grok 4.6 <noreply@x.ai>
timsehn
force-pushed
the
fix/clustered-pk-update-hook
branch
from
August 26, 2026 11:40
2612321 to
6694f8d
Compare
Collaborator
Author
|
CI Cause: Fix in 6694f8d: keep |
Commit: SummaryCoverage spans core data-change behavior, including inserts, updates, deletes, key changes, nested and cross-database writes, bulk transfers, and row identity reporting. It also exercises edge cases and concurrency, verifying correct notifications under overlapping operations and silence where notifications are not expected. Safe to merge — the exercised behaviors completed successfully with no PR-attributable regressions or unresolved failures, leaving no merge blocker identified. Tests run by Ito
Tip Reply with @itoqa to send us feedback on this test run. |
Contributor
DoltLite source coverage
Merged 201 pooled raw profiles from the distributed Linux correctness jobs. Per-file coverage (97 files)
|
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
1 participant
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.
Summary
sqlite3_update_hooknow fires for auto-clustered non-INTEGER PRIMARY KEY tables (for exampleTEXT PRIMARY KEY), using the same synthetic SQLrowidasSELECT rowid/last_insert_rowid().Explicit
WITHOUT ROWIDstays silent, matching stock SQLite.INTEGER PRIMARY KEY is unchanged (still
OP_Insert/OP_Delete).Why
DoltLite stores those tables as WITHOUT ROWID (
VisibleRowidstill set). Writes usedOP_IdxInsert, which had no update-hook, andOP_Deleteonly called the hook whenHasRowid.Tests
clustered_pk_update_hook: INTEGER PK events, TEXT PK INSERT/UPDATE/DELETE with matching rowids, explicit WITHOUT ROWID silent.hook.test34/34.Fixes #2427.