Skip to content

fix(storage): keep primary keys findable when one checkpoint splits a slot twice - #1513

Merged
adsharma merged 1 commit into
LadybugDB:mainfrom
zahariash:fix/pk-split-dangling-overflow-slot
Nov 5, 2026
Merged

adsharma merged 1 commit into
LadybugDB:mainfrom
zahariash:fix/pk-split-dangling-overflow-slot

Conversation

@zahariash

Copy link
Copy Markdown
Contributor

This work was produced with the help of language models.

Fixes #1510.

Copying 990,442 STRING keys into one table with commits at 141,501 rows and then every 250,000 left 5 keys unreachable on 0.22.1: MATCH (n:G {id: $k}) RETURN count(n) returned 0. With commits every 250,000 rows from the start, no key was lost. A smaller load with keys chosen by hash loses 9 of 18 keys on 0.18.0, 0.22.1 and main. There a scan still returns all 600 rows, and a rel COPY fails with Copy exception: Unable to find primary key value long-enough-primary-key-229740. Inserting a lost key again succeeds, which leaves two rows with one primary key, also after a reopen. INT64 keys lose entries too.

Fix. HashIndex::splitSlots buffers new overflow slots in a vector. One checkpoint can grow an index far enough to split a slot created earlier in the same pass, so the chain being read sits in the buffer being appended to. A reallocation left originalSlot on freed memory, whose first bytes hold the slot's fingerprints and get overwritten by the allocator, so entries moved with wrong fingerprints. The buffer is now a std::deque, whose elements stay in place as it grows.

The on-disk format is unchanged. A database built before this fix keeps its lost entries until the table is reloaded, and keeps stale copies of moved entries in the old chains. A growing checkpoint over those finishes on the 0.22.1 release build, with the other keys still found, and fails newSlotId == newSlotIterator.idx() on a runtime-checks build, this fix included.

Tests. SplitOfChainInNewOverflowSlotsKeepsKeysReachable picks keys by hash so that the second of two checkpoints moves 18 of them twice, then looks up all 600 keys after a reopen. On main 9 lookups return 0. That failure depends on what the allocator writes into freed memory and has only been run with glibc. The test runs at the default 4 KiB page only, since its keys are chosen for an index starting at 16 slots. Full ctest: 3358/3358.

Cost. Only checkpoints that grow an index take this path. A single-threaded 400,000-key load with commits at 140,000 and 390,000 rows costs 1,890 more mallocs on 2,236,783, and its instruction count stays within repeat noise (runtime-checks builds).

The issue's separate failure with an 8 GiB buffer_pool_size isn't touched here and needs its own issue.

… slot twice

HashIndex::splitSlots buffers new overflow slots in a vector. One checkpoint can grow a
sub-index far enough to split a slot created earlier in the same pass. That slot's overflow
chain lives in the buffer, so appending to the buffer can reallocate it while originalSlot
still points into it, and the pass goes on reading freed memory.

The allocator's metadata overwrites the start of the freed slot, which holds its fingerprints,
so entries move to the new chain with wrong fingerprints: lookups miss them while the rows stay
intact. Invalidating an entry in the freed slot is lost as well, which leaves a stale copy in the
old chain. A lost key can be inserted again, leaving two rows with one primary key. Every key
type is affected, since the slot layout and the fingerprint check are shared.

A deque keeps its elements in place when it grows at the end.

The trigger is the growth between checkpoints. 400,000 keys with commits at 140,000 and 390,000
rows lost 7 keys on b247d46 and on 0.22.1. On 0.22.1, commits at 200,000 and 400,000 lost none
and one commit lost none. The regression test loses the same 9 of 18 chosen keys on 0.18.0,
0.22.1 and b247d46.

A database built before this fix keeps its lost entries until the table is reloaded, and keeps
the stale copies. A growing checkpoint over those finishes on the 0.22.1 release build, with the
other keys still found. On a runtime-checks build it fails `newSlotId == newSlotIterator.idx()`.
That check stays a DASSERT: throwing in release would stop checkpoints that work today.

Cost on a runtime-checks build, single-threaded 400,000-key load with those commits, against
b247d46: 7,505.5M vs 7,507.0M instructions (repeats differ by 1.7M), 1,890 more mallocs on
2,236,783. A load in one commit never builds this buffer.

Fixes LadybugDB#1510

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sebastianbluhm

Copy link
Copy Markdown

I ran into the same bug independently on a different load and found the same cause: the std::vector buffer in splitSlots. I applied the same deque change to v0.18.1. With a fresh load the build passes with runtime checks on, and all 742,885 keys resolve through the PK index when read with the unpatched 0.18.1 wheel. So I can confirm the fix.

@adsharma
adsharma merged commit e382321 into LadybugDB:main Nov 5, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: COPY into an empty STRING-PK node table, after other tables in the same process, deterministically drops one key from the primary-key index

3 participants