fix(merge): clean secondary indexes after conflict-resolved merge#1033
Merged
Conversation
Three-way merge of a table with secondary indexes used to leak the rejected side's index entries when a conflict row resolved as ours's value. Two interlocking bugs: 1. mergeCatalogPass1 ran a STANDALONE three-way diff over each index tree, independent of the table-level merge. That path saw theirs's (v=2, rowid=1) as a benign RIGHT_ADD — different sort-key from ours's (v=3, rowid=1) — and dropped it into the merged index even though the row's table value resolved to ours. 2. The INLINE index merge driven by the table row-merge (which DOES skip conflict rows) wrote entries with a malformed key/value format: for IPK tables the row record stores the IPK column as NULL (a ghost placeholder, since the real value lives in the intkey), so buildIndexSortKey produced a sort key with 1 byte of NULL marker where the native index format has the 9-byte rowid sortkey. The inline result couldn't be substituted for the standalone result without breaking covering-index scans. Fix: - buildIndexSortKey now takes intKey + iPKey and substitutes the intKey value for the IPK ghost field, matching the native index format written by sortKeyFromIntRecordLocal in prolly_btree.c (key = sortkey(indexed_cols, IPK), value = empty). - Non-indexed columns are no longer included in the synthesized key — they aren't part of the native format either. - mergeCatalogPass1 now collects inline-merged index roots across iterations and applies them at the end of the pass, overriding the standalone results. This matches the architecture used by Dolt itself (no standalone three-way index merge; indexes flow from table row events). Verified against the native bytes captured from the SQL INSERT path for both single- and multi-column tables. Known limitation surfaced: dolt_conflicts_resolve --theirs writes theirs's row via doltliteApplyRawRowMutation, which bypasses index maintenance. The merged-table state is correct but the secondary index keeps ours's entry for the resolved row. Separate bug at the conflict-resolve layer; not addressed here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…--theirs Extends the merge-time index fix to cover the --theirs path. Before, dolt_conflicts_resolve --theirs called doltliteApplyRawRowMutation, which wrote theirs's row to the table tree only — secondary indexes kept the entry from ours's value (which the merge had taken for the conflict row), so after the resolve the index pointed at a stale row value. doltliteApplyRawRowMutation now: - Reads the row's current value from the table tree before mutating - Iterates pTab->pIndex, building (old, new) sort keys via the same doltliteBuildIndexSortKey helper the merge uses - Applies the old-key DELETE + new-key INSERT against each index's prolly tree The static-to-public refactor: doltliteBuildIndexSortKey, doltliteIpkSerialType, and doltliteIpkWriteBE move from file-static in doltlite_merge.c to extern; declared in doltlite_internal.h. prolly_btree.c can't include that header (TableEntry/SchemaEntry shape conflict with its local definitions), so it forward-declares the one symbol it needs locally. Test (doltlite_merge_index_conflict.sh) covers the previously-stale --theirs case (theirs_iv_three_rows), bringing the suite to 9 cases. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ve-index-cleanup # Conflicts: # test/run_doltlite_tests.sh
Sysbench-Style Benchmark: Doltlite vs SQLiteIn-MemoryReads
Writes
File-BackedReads
Writes
File-Backed (autocommit)Each statement runs as its own transaction — exposes per-commit ReadsReads have no commit cost; these are the same SQL files as the
Writes
100000 rows, median of 5 invocations per test, workload-only timing via host monotonic clock when available. Performance Ceiling Check (6x individual, 5x average)All tests within ceilings. |
Sysbench-Style Benchmark (composite PK): Doltlite vs SQLiteCompanion to the classic Sysbench-Style Benchmark. Every workload here In-MemoryReads
Writes
File-BackedReads
Writes
File-Backed (autocommit)Each statement runs as its own transaction — exposes per-commit ReadsReads have no commit cost; these are the same SQL files as the
Writes
100000 rows, median of 5 invocations per test, workload-only timing via host monotonic clock when available. Performance Ceiling Check (6x individual, 5x average)All tests within ceilings. |
Sysbench-Style Benchmark (BLOB PK): Doltlite vs SQLiteCompanion to the classic Sysbench-Style Benchmark. Every workload here In-MemoryReads
Writes
File-BackedReads
Writes
File-Backed (autocommit)Each statement runs as its own transaction — exposes per-commit ReadsReads have no commit cost; these are the same SQL files as the
Writes
100000 rows, median of 5 invocations per test, workload-only timing via host monotonic clock when available. Performance Ceiling Check (6x individual, 5x average)All tests within ceilings. |
Sysbench-Style Benchmark (TEXT PK): Doltlite vs SQLiteCompanion to the classic Sysbench-Style Benchmark. Every workload here In-MemoryReads
Writes
File-BackedReads
Writes
File-Backed (autocommit)Each statement runs as its own transaction — exposes per-commit ReadsReads have no commit cost; these are the same SQL files as the
Writes
100000 rows, median of 5 invocations per test, workload-only timing via host monotonic clock when available. Performance Ceiling Check (6x individual, 5x average)All tests within ceilings. |
For tables created as INT PRIMARY KEY (or any single-column PK that isn't INTEGER), SQLite/doltlite represent the PK as a pseudo-INDEX exposed in pTab->pIndex with idxType == SQLITE_IDXTYPE_PRIMARYKEY. That pseudo-index's tnum equals the table's own root. After the merge-index-cleanup PR, pass1 was collecting it into aIdxInfo and the inline merge built empty-valued entries for it. The end-of-pass patch then overwrote the table's root with the empty-valued index root — rows merged in from the other branch showed NULL non-PK columns post-merge. Detected by vc_oracle_at_test.sh's at_head_after_merge: CREATE TABLE t(id INT PRIMARY KEY, v INT); INSERT (1,10),(2,20); commit; branch feat; INSERT (3,30); commit; checkout main; INSERT (4,40); commit; merge feat; → row 3 came back as (3, NULL) instead of (3, 30). Fix: skip primary-key pseudo-indexes when populating aIdxInfo so they aren't treated as secondary indexes by the inline merge. Also resolves a leftover stash conflict in src/chunk_store.h that predated this branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Same WITHOUT ROWID pseudo-index trap as the merge-path fix in the previous commit, in the conflict-resolve path. doltliteApplyRawRowMutation iterates pTab->pIndex to update secondary indexes after the table mutation, but for INT-PRIMARY-KEY (auto-converted to WITHOUT ROWID) tables the PK appears as a pseudo-INDEX with tnum == table's root. Treating it as a secondary index built an empty-valued entry and overwrote the table tree, dropping the non-PK column values. Detected by vc_oracle_conflicts_resolve_test.sh: 10 --theirs cases failed with NULL where theirs's value should be (e.g. resolve_theirs_single_table got 'R|1|' instead of 'R|1|200'). Fix: skip pIdx->idxType == SQLITE_IDXTYPE_PRIMARYKEY in the applyRawRowMutation index-maintenance loop, mirroring the merge fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
For WITHOUT ROWID tables, the row record stored in the table tree
contains only the non-PK columns — the PK columns live in the b-tree
key. doltliteBuildIndexSortKey was building the index key from the
row record alone, which dropped the PK entirely; two rows with the
same indexed-column value but different PKs collided on the resulting
key and one overwrote the other in mi->pEdits.
This silenced UNIQUE-violation detection because the duplicate that
should have surfaced never made it into the index map (two inserts
with identical key in a MutMap is a replace, not a collision).
Detected by vc_oracle_fk_merge_test.sh's without_rowid_unique_merge_errors:
CREATE TABLE t(pk TEXT PRIMARY KEY, v1 INT UNIQUE);
INSERT ('base',10); commit; branch feat;
feat: INSERT ('feat',20); commit;
main: INSERT ('main',20); commit; merge feat;
→ Expected: 'constraint violations | rolled back'.
→ Got: success (silently corrupt UNIQUE index).
Fix: extend doltliteBuildIndexSortKey to accept (pTreeKey, nTreeKey).
For WITHOUT ROWID tables (iPKey < 0), append the table b-tree key
bytes — which are already a sortkey of the PK columns — to the
sortkey output. Matches the native iv format captured from the
SQL INSERT path:
iv key = sortkey(v1) + sortkey(pk)
Six callers in doltlite_merge.c and one in prolly_btree.c
(mutateSecondaryIndex) updated to pass pChange->pKey / pKey through.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Three-way merge over a table with secondary indexes used to leak the rejected side's index entries when a row-level conflict resolved as ours's value. After
dolt_conflicts_resolve --oursand commit, scans through the index returned phantom rows — e.g. anivcovering scan showed 4 entries for a 3-row table because theirs's stale entry for the conflict row was never filtered.Diagnosed by direct byte-comparison of the native iv INSERT path's output (
sortKeyFromIntRecordLocalinprolly_btree.c) versus whatbuildIndexSortKeyproduced in the merge path.Two interlocking bugs
1. Standalone three-way diff over each index tree.
mergeCatalogPass1iterated index entries as if they were tables and three-way merged each one independently from the parent table's row merge. The diff saw theirs's(v=2, rowid=1)as a benignRIGHT_ADD— different sort key from ours's(v=3, rowid=1)— and added it to the merged index. The index merge had no concept of "table-level conflict resolved to ours."2. Format mismatch in the inline index merge. The TABLE row-merge callback (which correctly skips conflict rows) wrote index edits via
buildIndexSortKey. For IPK tables, the row record stores the IPK column as NULL (a ghost placeholder — the actual rowid lives in the b-tree intkey).buildIndexSortKeyoperated on the row record directly, so it produced a sort key with a 1-byte NULL marker where the native index format has a 9-byte rowid sortkey. The inline-merged root had keys that didn't match what covering-index scans expected.Fix
buildIndexSortKeynow takesintKey+iPKeyand substitutes the intKey value for the IPK ghost field, matching the native format fromsortKeyFromIntRecordLocal.mergeCatalogPass1collects inline-merged index roots across iterations and applies them at the end of the pass, overriding the standalone results.This matches Dolt's own architecture in
go/libraries/doltcore/merge/: no standalone three-way index merge; secondary indexes flow exclusively from table row events; conflict rows skip the secondary-index path so ours's entries remain.Test plan
test/doltlite_merge_index_conflict.sh— 8 cases: conflict + --ours (single index), --theirs (table state only — see limitation below), non-conflict merge, two indexes on one table with conflict on a different column, DELETE-vs-MODIFY conflict resolved --oursdoltlite_merge.sh(91 cases),doltlite_conflicts.sh(40),doltlite_schema_merge.sh(76),doltlite_savepoint.sh(128),doltlite_conflict_rows.sh(29) all passKnown limitation
dolt_conflicts_resolve --theirswrites theirs's row viadoltliteApplyRawRowMutation, which only updates the table — secondary indexes are not maintained. The merged table state is correct but the index keeps ours's stale entry for the resolved row. This is a separate pre-existing bug at the conflict-resolve layer; the fix would be to thread index maintenance throughdoltliteApplyRawRowMutation. Tracked separately.🤖 Generated with Claude Code