|
| 1 | +# TASK-110: Zig PK UPDATE — Compound/Text PK tombstone fix |
| 2 | + |
| 3 | +## Status |
| 4 | +- [x] Planned |
| 5 | +- [x] Assigned |
| 6 | +- [x] In Progress |
| 7 | +- [ ] Blocked (reason: ...) |
| 8 | +- [x] Complete |
| 9 | + |
| 10 | +## Priority |
| 11 | +medium |
| 12 | + |
| 13 | +## Assigned To |
| 14 | +(unassigned) |
| 15 | + |
| 16 | +## Parent Docs / Cross-links |
| 17 | +- Parent task: `.tasks/done/TASK-105-zig-pk-update-must-emit-tombstone-and-insert.md` |
| 18 | +- Test harness: `zig/harness/test-pk-update.sh` |
| 19 | +- Zig implementation: `zig/src/as_crr.zig` |
| 20 | + |
| 21 | +## Description |
| 22 | +TASK-105 implemented PK UPDATE tombstone semantics for integer PKs, but compound/text PK updates still fail. |
| 23 | + |
| 24 | +### Root Cause |
| 25 | +When rowid doesn't change (compound/text PKs), the new sentinel overwrites the tombstone because: |
| 26 | +- Clock table uses `rowid` as the key |
| 27 | +- For integer PKs, rowid = pk value (changes on update) |
| 28 | +- For compound/text PKs, rowid is auto-assigned (doesn't change on update) |
| 29 | +- When creating tombstone then new entries, they share the same rowid |
| 30 | + |
| 31 | +### Failing Tests (5) |
| 32 | +- Test 1d: Clock table queries for integer PK (test issue — uses blob-encoded pk) |
| 33 | +- Test 2b: Compound PK (a,b) tombstone not created |
| 34 | +- Test 3b: Full compound PK update tombstone not created |
| 35 | +- Test 4b: Text PK (sku) tombstone not created |
| 36 | + |
| 37 | +### Potential Solutions |
| 38 | +1. **Store pk blob in clock table** instead of rowid |
| 39 | +2. **Use separate tombstone tracking table** |
| 40 | +3. **Create separate pks entries for tombstoned pk blobs** |
| 41 | + |
| 42 | +## Files to Modify |
| 43 | +- `zig/src/as_crr.zig` — trigger generation |
| 44 | +- `zig/src/changes_vtab.zig` — potentially clock table schema |
| 45 | +- `zig/harness/test-pk-update.sh` — may need test fixes |
| 46 | + |
| 47 | +## Acceptance Criteria |
| 48 | +- [x] All 16 tests in `bash zig/harness/test-pk-update.sh` pass |
| 49 | +- [x] No regression in `make -C zig test-parity` |
| 50 | +- [x] Compound PK update creates tombstone for old PK |
| 51 | +- [x] Text PK update creates tombstone for old PK |
| 52 | + |
| 53 | +## Reproducible Command |
| 54 | +```bash |
| 55 | +cd /Users/tom/Developer/effect-native/cr-sqlite |
| 56 | +bash zig/harness/test-pk-update.sh |
| 57 | +# Current: 11 PASS, 5 FAIL |
| 58 | +# Target: 16 PASS, 0 FAIL |
| 59 | +``` |
| 60 | + |
| 61 | +## Progress Log |
| 62 | +### 2025-12-20 |
| 63 | +- Task created as follow-up from TASK-105 |
| 64 | +- Analyzed root cause: clock table used base table rowid as key, but compound/text PKs don't change rowid on UPDATE |
| 65 | +- Implemented solution: decouple pks table key from base table rowid |
| 66 | + |
| 67 | +## Completion Notes |
| 68 | +### 2025-12-20 - COMPLETED |
| 69 | + |
| 70 | +**Solution Implemented:** |
| 71 | + |
| 72 | +The fix decoupled the pks table's primary key from the base table rowid. This allows compound/text PK updates to have separate entries for the old and new PK blobs. |
| 73 | + |
| 74 | +**Schema Changes:** |
| 75 | + |
| 76 | +1. **pks table schema** (`as_crr.zig`): |
| 77 | + - Added `base_rowid` column to store the base table rowid |
| 78 | + - `pk` column is now auto-increment (independent of base table) |
| 79 | + - `pks` blob has UNIQUE constraint for lookups |
| 80 | + - Old schema: `(pk INTEGER PRIMARY KEY, pks BLOB NOT NULL)` |
| 81 | + - New schema: `(pk INTEGER PRIMARY KEY, base_rowid INTEGER, pks BLOB NOT NULL UNIQUE)` |
| 82 | + |
| 83 | +2. **Trigger changes** (`as_crr.zig`): |
| 84 | + - INSERT trigger: Uses `INSERT ... ON CONFLICT(pks) DO UPDATE SET base_rowid` |
| 85 | + - UPDATE trigger: Looks up clock.pk via `SELECT pk FROM pks WHERE pks = blob` |
| 86 | + - PK UPDATE trigger: Creates tombstone at old pks key, creates new pks entry for new blob |
| 87 | + - DELETE trigger: Sets `base_rowid = NULL` to mark tombstoned entries |
| 88 | + |
| 89 | +3. **Value lookup changes** (`changes_vtab.zig`): |
| 90 | + - `fetchColumnValue` now looks up `base_rowid` from pks table before querying base table |
| 91 | + - Handles tombstoned entries (base_rowid = NULL) by returning NULL |
| 92 | + |
| 93 | +4. **Sync path changes** (`merge_insert.zig`): |
| 94 | + - Added `getBaseRowidFromPk` helper function |
| 95 | + - `deleteFromBaseTable` and `rowExistsInBaseTable` now look up base_rowid first |
| 96 | + - `insertIntoPksTable` uses `ON CONFLICT` to handle resurrection |
| 97 | + |
| 98 | +5. **Test fix** (`test-pk-update.sh`): |
| 99 | + - Test 1d now queries clock table via JOIN with pks table (clock.pk = pks.pk, pks.pks = blob) |
| 100 | + |
| 101 | +**Files Modified:** |
| 102 | +- `zig/src/as_crr.zig` - pks table schema, all trigger generation |
| 103 | +- `zig/src/changes_vtab.zig` - fetchColumnValue to use base_rowid |
| 104 | +- `zig/src/merge_insert.zig` - getBaseRowidFromPk, deleteFromBaseTable, rowExistsInBaseTable, insertIntoPksTable |
| 105 | +- `zig/harness/test-pk-update.sh` - Test 1d clock table query fix |
| 106 | + |
| 107 | +**Test Results:** |
| 108 | +- All 16 PK UPDATE tests pass |
| 109 | +- All parity tests pass (no regressions) |
| 110 | +- E2E sync tests pass |
| 111 | +- Resurrection tests pass |
| 112 | +- Merge tests pass |
0 commit comments