Skip to content

feat(store): publish splayed replacements atomically - #649

Open
belowzeroff wants to merge 2 commits into
RayforceDB:devfrom
belowzeroff:feat/atomic-splay-generations
Open

belowzeroff wants to merge 2 commits into
RayforceDB:devfrom
belowzeroff:feat/atomic-splay-generations

Conversation

@belowzeroff

Copy link
Copy Markdown
Contributor

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

  • Add generation-based publication for splayed tables:
    • replacements are staged under .generations/<generation-id>/;
    • .current is updated only after the staged table is complete;
    • readers resolve through .current.
  • Keep legacy splayed directories readable when .current is absent, so existing tables continue to work before their first replacement.
  • Serialize writers with a per-table .write.lock.
  • Build staged table indexes before publishing the new generation.
  • Route CSV and partitioned-table splay writes through the same atomic publication path.
  • Document the new layout, compatibility behavior, and operational caveats.
  • Add C and RFL coverage for:
    • atomic publish behavior;
    • failed replacement preserving the old active generation;
    • invalid manifest handling;
    • mmap/readers keeping old generation files valid;
    • CSV/partitioned write paths.

User-visible behavior

  • Existing splayed tables without .current remain readable without migration.
  • After a replacement, readers see either the previous full table or the newly published full table.
  • Failed or interrupted writes leave the previously published table active.
  • Old generation directories are intentionally retained so existing mmap/readers are not broken; automatic GC is not included in this PR.
  • Once a table has been rewritten with this layout, older binaries that do not understand .current should 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 release
  • git diff --check origin/dev...HEAD

I 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.

@belowzeroff
belowzeroff force-pushed the feat/atomic-splay-generations branch 2 times, most recently from 602a279 to 672400b Compare September 29, 2026 16:29
@belowzeroff
belowzeroff force-pushed the feat/atomic-splay-generations branch from 672400b to 235db03 Compare September 29, 2026 17:10

@singaraiona singaraiona left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 .current manifest.
  • Symbols and readers: the writer flushes the symfile before publishing, and readers resolve .current before 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: .current accepts 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 .current and 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=1 means cppcheck can never fail, even when run by hand.
  • CI now checks only changed files.
  • group.c, query.c and agg_engine.c are 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_files fsyncs every file again after ray_col_save already fsynced it; only the files that ray_splay_build_indexes appended to need the second sync.
  • Windows publish failure. MoveFileExA(..., MOVEFILE_REPLACE_EXISTING) fails while a reader has .current open (CRT fopen doesn't share delete), and there is no retry, so publish can fail under concurrent reads there.
  • NFS. .write.lock relies on flock, 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.

@singaraiona

Copy link
Copy Markdown
Collaborator

Following up on point 3 of the review: the cppcheck slowdown is fixed separately in #654. The root cause was cppcheck's #ifdef configuration enumeration: most files were analysed up to 12 times. Pinning one configuration makes the whole tree run 4–5x faster, while keeping every file and --error-exitcode=1. So this PR can drop its ci.yml and Makefile changes.

singaraiona added a commit that referenced this pull request Sep 30, 2026
#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.

This branch has not been deployed

No deployments
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