fix: maintain secondary index for delete set null - #27622
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
aptend
left a comment
There was a problem hiding this comment.
Requesting changes because the generic SET NULL shortcut violates composite secondary-index storage semantics. The composite PRE_INSERT_SK path deliberately uses serial_full/SerialWithoutCompacted and retains rows whose key has NULL parts. At this exact head, I changed the new integration check to require the hidden row to remain and reproduced expected 3, actual 2; the existing composite-null executor test passes. The original focused planner and integration tests pass, but they encode the incorrect hidden-row count and do not query the NULL key.
| if isUpdate && len(delCtx.fkSetNullColumns) > 0 { | ||
| for _, part := range indexdef.Parts { | ||
| if _, becomesNull := delCtx.fkSetNullColumns[catalog.ResolveAlias(part)]; becomesNull { | ||
| skipIndexInsert = true |
There was a problem hiding this comment.
skipIndexInsert now applies to every regular index containing the SET NULL column, but composite regular secondary indexes use serial_full and retain rows with NULL parts (SerialWithoutCompacted; its executor test explicitly expects all composite rows to remain). For the PR reproducer idx_parent(parent_id, note), deleting parent 100 must replace the old hidden key with serial_full(NULL, "s100-a"). This branch instead deletes it and returns, so the hidden count drops from 3 to 2 and SELECT ... FORCE INDEX(idx_parent) WHERE parent_id IS NULL AND note = "s100-a" cannot find the surviving child. Please restrict the skip to index representations that compact NULL (unique indexes and the current single-part SK representation), while retaining delete + PRE_INSERT_SK for composite regular indexes.
There was a problem hiding this comment.
Fixed in c8f09d3. The planner now counts logical index parts without the appended primary-key alias: single-part secondary indexes still compact a NULL key, while composite secondary indexes delete only the matched old hidden row and rebuild the NULL-containing serial_full key from an independent FK action stream. Added forced-index checks for both old and NULL composite keys, literal and prepared execution, rollback, and failed-delete atomicity.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review of exact head b30684c. The planner change removes the reported panic, but it corrupts composite non-unique secondary-index maintenance.
[P1 correctness] Preserve the replacement hidden row for composite secondary indexes with NULL components.
The new skipIndexInsert rule becomes true whenever any SET NULL column appears in indexdef.Parts, regardless of index kind or arity. That conflates two different storage contracts:
- UNIQUE and single-column non-unique index paths compact rows whose indexed value is NULL.
- Composite non-unique secondary indexes use PRE_INSERT_SK plus SerialWithoutCompacted/serial_full specifically to retain every base row, including partial-NULL and all-NULL keys.
For the issue schema key idx_parent(parent_id, note), changing parent_id from 100 to NULL must therefore delete serial_full(100, s100-a) and insert serial_full(NULL, s100-a). This PR deletes the old hidden row and returns before PRE_INSERT_SK, leaving the child row with no corresponding index row.
I reproduced this against the exact head with the PR embedded test: changing the post-delete hidden-table oracle from 2 to the contractually correct 3 fails deterministically with expected 3, actual 2. The checked-in test currently encodes the corruption by requiring 2 and only querying parent_id = 100/200; it never proves that the replacement NULL composite key is reachable.
Please restrict the no-reinsert shortcut to storage forms that actually omit NULL rows: UNIQUE indexes and a single-column regular secondary index whose only key becomes NULL. Composite non-unique indexes must keep the existing delete plus PRE_INSERT_SK reinsertion path. Update the planner oracle accordingly and add public execution assertions for parent_id IS NULL with the remaining composite part, hidden-row count 3, literal/prepared execution, and rollback/failed-delete atomicity.
The change is otherwise focused and CI is green; this confirmed index-state corruption is the blocker.
aunjgr
left a comment
There was a problem hiding this comment.
Reviewed exact head b30684cf8ce027e62d80fbe9988132ab011fadba against base 5bc051abc44f34ccb6813e2a0bb5585d5a1db67e.
The generic skipIndexInsert rule corrupts composite non-unique secondary indexes. It becomes true when any SET NULL column appears in indexdef.Parts, but composite regular indexes use PRE_INSERT_SK and SerialWithoutCompacted / serial_full specifically to retain rows whose key contains NULL.
For the checked-in schema idx_parent(parent_id, note), deleting parent 100 must delete hidden key serial_full(100, s100-a) and insert serial_full(NULL, s100-a). This head deletes the old row and returns before PRE_INSERT_SK, leaving the surviving child without its hidden index row. The integration test encodes that corruption by expecting the hidden count to fall from 3 to 2 and never querying the NULL key.
Restrict the no-reinsert path to index storage forms that actually omit the resulting NULL key. Composite non-unique secondary indexes must retain delete plus PRE_INSERT_SK reinsertion. Change the hidden-row oracle to 3 and assert the NULL/remaining-part row through the forced index for literal and prepared execution, with rollback/failure atomicity controls.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review of exact head c8f09d377b144cd95c882f768027d44ba33a0b1f against merge base 1891117aedce0419a81630b3011906b560b93d0a completed. The revision fixes the prior composite-secondary-index corruption by distinguishing NULL-compacting UNIQUE/single-column secondary forms from composite serial_full storage. Composite SET NULL now deletes only the matched old hidden row and independently rebuilds the replacement key from the FK action image; positional row-id/PK projection is explicit, so unmatched action rows cannot leak into deletion and the insert branch is not starved by a shared stream. Exact-head integration coverage proves the NULL composite key is reachable through the forced index, hidden-row cardinality remains three, literal and prepared deletes agree, rollback restores state, and a failed parent delete is atomic. The no-index and single-column compacting controls remain covered. I found no remaining correctness, performance, or unhappy-path blocker; exact-head CI is green.
What type of PR is this?
Which issue(s) this PR fixes:
issue #27539
What this PR does / why we need it:
fix: maintain secondary index for delete set null