feat(store): publish splayed replacements atomically - #649
belowzeroff wants to merge 2 commits into
Conversation
602a279 to
672400b
Compare
672400b to
235db03
Compare
singaraiona
left a comment
There was a problem hiding this comment.
Reviewed at 235db035. The problem is real and worth solving: on dev, replacing a splayed table renames the new columns in one at a time. A crash or I/O error partway through, or a reader loading at the wrong moment, can end up with old and new columns mixed. When the columns are the same length, nothing detects it. The old comment in splay.c admitted as much.
The core protocol is sound, and I checked the parts that matter:
- Publishing: the new table is written in full to a fresh
.generations/<id>/, then made live by atomically renaming the one-line.currentmanifest. - Symbols and readers: the writer flushes the symfile before publishing, and readers resolve
.currentbefore revalidating the cached domain. A reader never sees a code it can't resolve, and every partition still shares one domain object. - Indexes: built in the staged generation before publication, instead of appended to live files after the save.
- Manifest validation:
.currentaccepts only[0-9g-]names under.generations/, so a tampered manifest can't point outside the table. - Tests: real failures are forced (a read-only root), and every public entry point is covered.
- Scope: I found no other reader or writer of splayed dirs that bypasses the resolution.
Requesting changes for three reasons:
1. Retention is unbounded. Every replacement keeps a full copy forever, and a failed write leaves its half-written generation behind. I measured five .db.splayed.set calls to the same path, 5M rows × 4 columns: 788 MB on this PR vs 158 MB on dev. Write-once HDB partitions are unaffected, but anything saved repeatedly (RDB snapshots, .db.parted.fill, re-running a CSV conversion) fills the disk.
- The reason given for retention is mostly wrong on POSIX: unlinking a file doesn't invalidate existing mmaps. The only exposure is the short window between a reader resolving
.currentand opening its columns. - So "keep current + previous, unlink older" is safe. Unpublished (failed) generations can go immediately. On Windows, where mapped files can't be deleted, skip them on failure and retry on the next publish.
2. Older binaries silently read stale data. I wrote a table twice with this PR, then read it with a dev binary: it returned the first version ([1 2 3] instead of [100 200]), with no error. The docs say not to use older binaries, but a rollback, a second tool or a mixed fleet will get old data quietly. A cheap mitigation: at the first staged publish, rename the root .d (the new binary no longer reads it once .current exists), so legacy readers fail loudly instead of being silently wrong.
3. The CI/Makefile changes are unrelated and weaken cppcheck.
- Dropping
--error-exitcode=1means cppcheck can never fail, even when run by hand. - CI now checks only changed files.
group.c,query.candagg_engine.care skipped by default.- The motivation, cppcheck hanging in CI, is legitimate, but please split it into its own PR with that trade-off stated.
Should fix:
- Double fsync. Durable saves are about 8% slower in the same benchmark (≈870 vs ≈805 ms per save).
splay_sync_filesfsyncs every file again afterray_col_savealready fsynced it; only the files thatray_splay_build_indexesappended to need the second sync. - Windows publish failure.
MoveFileExA(..., MOVEFILE_REPLACE_EXISTING)fails while a reader has.currentopen (CRTfopendoesn't share delete), and there is no retry, so publish can fail under concurrent reads there. - NFS.
.write.lockrelies onflock, which isn't reliable on NFS; worth a line in the docs.
Worth considering, not blocking: publishing by atomically renaming a symlink (table path → current generation) instead of a manifest file. Older binaries follow symlinks, so they'd read the current data, which removes item 2 entirely. The costs are symlink privileges on Windows and relocating a standalone table's .sym out of the generation directory.
Happy to re-review once retention is bounded, legacy readers fail loudly, and the CI changes are split out. None of that needs a redesign of the core.
|
Following up on point 3 of the review: the cppcheck slowdown is fixed separately in #654. The root cause was cppcheck's |
#654) The advisory static-analysis job has taken 40-50 minutes on nearly every run. The cause is cppcheck's configuration enumeration: given no -D, it analyses each file once per combination of the #ifdef branches it finds (platform, DEBUG, endianness, fuzzing), up to 12, and most files hit that cap ("Too many #ifdef configurations"). CPPCHECK_DEFS pins the gcc / x86-64 / Linux debug configuration CI compiles. Measured with cppcheck 2.13.0 (the version the runner installs): format.c 449 s -> 57 s, eval.c 568 s -> 78 s, pivot.c 325 s -> 42 s, and the whole tree at -j 4 in 632 s, exit 0. Findings are unchanged: the single-file cross-TU warning in eval.c still reports with the pin. Unlike the CI half of #649, this keeps what made the job worth running: the whole tree (no changed-files-only pass, no skipped translation units) and --error-exitcode=1, so a finding still fails the advisory job. What the pin gives up is analysis of the Windows/macOS/WASM #ifdef branches, which were only ever sampled within the 12-configuration cap anyway. The job also gets a 30-minute timeout so a regression in analysis time cannot hold a runner for hours.
Why
Splayed table replacement currently rewrites files in place. For production-style HDB/RDB usage this is risky: a concurrent reader or a process crash can observe a partially replaced table, mixed old/new columns, or stale metadata/index files.
This PR makes splayed-table replacement snapshot-oriented: readers should either see the old complete table or the new complete table, never an in-between filesystem state.
What changed
.generations/<generation-id>/;.currentis updated only after the staged table is complete;.current..currentis absent, so existing tables continue to work before their first replacement..write.lock.User-visible behavior
.currentremain readable without migration..currentshould not be used to read that table.Validation
make test TEST_FILTER=splay TEST_CORES=0 DEBUG_CFLAGS='-fPIC -Wall -Wextra -Werror -Wstrict-prototypes -Wno-unused-parameter -std=c17 -g -O0 -march=native -DDEBUG -fno-omit-frame-pointer' DEBUG_LDFLAGS=''make test TEST_FILTER=atomic_generations TEST_CORES=0 DEBUG_CFLAGS='-fPIC -Wall -Wextra -Werror -Wstrict-prototypes -Wno-unused-parameter -std=c17 -g -O0 -march=native -DDEBUG -fno-omit-frame-pointer' DEBUG_LDFLAGS=''make releasegit diff --check origin/dev...HEADI also tried the default sanitizer debug path locally, but this runner is missing
/usr/lib64/libasan.so.6.0.0; the build reached the final link step before failing on that environment dependency.