Skip to content

fix(storage): refresh node group scan state after a checkpoint - #1094

Merged
adsharma merged 2 commits into
LadybugDB:mainfrom
kory-io:fix/refresh-scan-state-after-checkpoint
Oct 4, 2026
Merged

adsharma merged 2 commits into
LadybugDB:mainfrom
kory-io:fix/refresh-scan-state-after-checkpoint

Conversation

@kory-io

@kory-io kory-io commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

Description

Bug. Read-only transactions are not blocked by a checkpoint, so a checkpoint can run between two vectors of the same node table scan (or between two lookups sharing a scan state). NodeGroup::checkpoint merges all chunked groups into a single persistent one and rewrites its column chunk metadata, but the running scan's NodeGroupScanState still holds the chunked group index and the per-segment metadata (page ranges, compression metadata, dictionary sizes) captured by initializeScanStateForChunkedGroup before the checkpoint. NodeGroup::scan then reads the checkpointed pages with stale metadata, or indexes past the end of the chunked group list.

Symptoms: scans return corrupted values with no error (strings assembled from the wrong dictionary offsets, wrong integers), and a scan that was inside a committed in-memory chunked group crashes. Reproduces on main (v0.21.2) with 3 scanning connections, a delete/re-insert writer and a CHECKPOINT loop at 8 threads: garbled strings in 3 of 8 two-minute runs.

Fix. Count the checkpoints of a node group (NodeGroup::numCheckpoints, protected by the chunked-groups lock) and record the count in the scan state when it is initialized. scan, scanInternal, lookup and lookupMultiple, which already hold that lock, re-initialize the state against the checkpointed group when the counts differ. Row indices within a node group are stable across a checkpoint, so nextRowToScan is kept. Cost: one integer comparison per scanned vector / lookup.

Tests. ScanAcrossCheckpointTest.CheckpointWhileScanningPersistentGroup and .CheckpointWhileScanningInMemoryGroup drive a NodeTableScanState in a read-only transaction and checkpoint between two vectors. Without the fix the first returns wrong values and the second segfaults; with it both pass. The stress loop above: 0 of 13 runs garbled with the fix.

Verified locally: release ctest 2854/2855 (the one failure, dictionary_bug~orb383_relationship_projection_obfuscated.AnonymousParquetDeleteReload, also fails on main); ASan + runtime checks: transaction_test 87/87, storage unit tests, e2e transaction~*:dml_node~*:dml_rel~*:storage* 725/725.

Not addressed here.

  • Rel-table (CSR) scans keep their own scan state across CSRNodeGroup::checkpoint and may have the same exposure.
  • Separately, an open read-only transaction can see commits newer than its snapshot once they are checkpointed. That looks like a design question rather than a small fix; happy to open an issue with a short repro if useful.

Types of changes

  • Bug fix

Read-only transactions are not blocked by a checkpoint, so a checkpoint
can run between two vectors of the same node table scan (or between two
lookups that share a scan state). NodeGroup::checkpoint merges all
chunked groups into a single persistent one and rewrites its column
chunk metadata, but the NodeGroupScanState of the running scan still
holds the chunked group index and the per-segment metadata (page
ranges, compression metadata, dictionary sizes) captured before the
checkpoint. The scan then reads the checkpointed pages with stale
metadata, or indexes past the end of the chunked group list.

This returned corrupted values (strings assembled from the wrong
dictionary offsets, wrong integers) without any error, and could crash
when the scan was inside a committed in-memory chunked group that the
checkpoint merged away.

Count the checkpoints of a node group and record the count in the scan
state when it is initialized. Scans and lookups, which already hold the
chunked-groups lock, re-initialize the state against the checkpointed
group when the counts differ. Row indices within a node group are
stable across a checkpoint, so the scan position is kept.
@adsharma

adsharma commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Does CSRNodeGroup need the same fix?

@adsharma
adsharma merged commit 8c2d52e into LadybugDB:main Oct 4, 2026
4 checks passed
@kory-io

kory-io commented Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

Yes. Rel scans snapshot their chunk state and CSR header once, and CSRNodeGroup::checkpoint does not bump the counter this PR checks, so that refresh never runs for them. A checkpoint also replaces the persistent group and drops the in-memory index, and CSR offsets can move, so the node-table approach does not carry over. I'll follow up with a separate test and fix.

@kory-io

kory-io commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

The CSR follow-up is #1116. A scan that is already running keeps the CSR state it started with. Checkpoint does not wait for it, and the node-table cursor refresh is not reused.

@kory-io
kory-io deleted the fix/refresh-scan-state-after-checkpoint branch October 5, 2026 22:46
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.

2 participants