Apply accumulated deletes during compaction and sync - #23
Merged
Merged
Conversation
Tombstoned rows were never being physically removed from large data files. `ducklake_merge_adjacent_files` skips any file already at the target size, so it only ever clears deletes from small files, and DuckLake's `rewrite_data_files` — the function that actually applies them — was never called at all. Its own default threshold is 0.95 (rewrite only when a file is 95% deleted), so even calling it naively would be close to a no-op. The backlog matters for more than disk: on every DELETE, DuckLake loads the full delete history of each data file it opens and holds it for the statement (ducklake_multi_file_reader.cpp), so accumulated deletes make later DELETEs progressively more expensive in memory regardless of how few rows they remove. - Add `pending_deletes()` reporting live delete files / tombstoned rows. - Add `compact(rewrite_deletes=<fraction>, max_files=N)` → `ducklake_rewrite_data_files`, run before the cleanup cycle so the superseded files get collected. `max_files` bounds one run, since rewriting real data is slow and a large backlog needs to be worked through incrementally. - Repeat the expire → merge → cleanup cycle to a fixed point: merging creates new snapshots and freshly-superseded files that only a later expire pass can collect, so one pass never fully compacts. - `podlake compact` gains --rewrite-deletes / --max-files and always reports the pending-delete backlog before and after. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ducklake_rewrite_data_files only gained max_compacted_files in newer DuckLake versions; on older ones passing it is a BinderException. Retry without the bound and warn that the whole backlog will be rewritten in one pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ducklake_rewrite_data_files returns one row per table/compaction group, so counting rows reported a constant (e.g. '14 files') regardless of the work done. Sum files_processed instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Leaving the delete-rewrite opt-in reproduces the footgun it was added to fix: a `compact` that reports success while silently leaving tombstoned rows, which make every later DELETE load more off-pool memory until deltas fail outright. - `compact` now applies deletes by default (--delete-threshold, default 0.1), with --no-rewrite-deletes for a quick disk-only pass. It warns before starting an unbounded rewrite of a large backlog, since that can't be interrupted for partial credit. - `sync` / `sync-all` gain --max-pending-deletes (default 150M rows, 0 disables) and apply the backlog mid-sync when it crosses that line. A long delta chain — a first-time load of ~300 resources, or duke's 136-delta catch-up — otherwise grows its own memory use until it dies partway through. The default is calibrated from a 32GB box (~105M fine, ~558M peaked at 95%), hence the flag. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The mid-sync trigger fired on every single resource: the default (150M) sat just above the floor the rewrite can actually reach, so each check found the backlog over the line, ran a full compaction cycle that reclaimed ~nothing (the residue lives in files under the 0.1 delete threshold), and left it over the line again. - Raise the default to 300M — clear of the ~150M floor observed after a cleanup, still well under the ~558M danger zone. - Add hysteresis: if a rewrite leaves the backlog above 75% of the trigger, raise the trigger to 1.5x what it actually left, so the next check has room to be useful instead of repeating a no-op cycle. Reported, not silent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The backoff raised the trigger 1.5x each time a rewrite came up short, so a backlog the rewrite can't keep up with would ratchet it past the range where deltas stop fitting in memory. Cap it at 2x the configured limit, and when it gets there say plainly that the backlog isn't being reclaimed and a lower --delete-threshold is needed — rather than silently tolerating a growing backlog. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A 1.1M-record delta was OOM-killed at 153M pending rows, on the same lake where a 1.8M-record one had succeeded at 76M — so cost per pending row is far steeper than assumed, and the previous defaults were unsafe: - 300M trigger was well above the level that actually kills a large delta. - The mid-sync rewrite was pinned at threshold 0.1, whose floor on this lake was ~157M — above the OOM point, so it could never have made the sync safe. - The backoff raised the trigger when a rewrite came up short, which defers the failure to a later, larger resource instead of preventing it. Now: trigger defaults to 75M; the mid-sync rewrite starts at 0.05 and halves down to 0.01 while the backlog stays over the limit; only once the threshold bottoms out is the backlog treated as irreducible, and that is reported plainly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…floor A 0.05 rewrite bottomed out at ~76M on the production lake, so a 75M trigger sits below what the rewrite can achieve and fires on every resource without ever getting under the line. The usable window there is ~76M-153M; 100M sits inside it. Document the tension so the number can be tuned per lake. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
What kills a sync is a large resource meeting a moderate backlog, not a high backlog on its own — the delete pays for the tombstones it creates *and* every tombstone already on the files it opens. Checking after each apply couldn't see what was coming next. Move the check before the apply, where the resource's size is already known from the ResourceSync manifest, and hold resources over 100MB to half the normal limit. Small deltas keep the looser limit, so the long tail costs nothing. Also record the irreducible floor once the rewrite bottoms out at the minimum threshold, so later resources stop paying for compactions that cannot help. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The old text framed compaction as a disk-space chore, which is how a 1.5B-row backlog accumulated unnoticed until deltas stopped fitting in memory. Give it its own section explaining what tombstones cost at write time, why merging small files doesn't clear them, how to read the reported backlog, and how sync keeps it in check. Also note that memory_limit bounds the buffer pool only, and make the routine cycle sync -> compact -> publish. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
apply_resource runs delete/insert/commit in one transaction with no progress of its own, so a stall or memory spike can't be attributed to a step. Log a marker before and after each statement at INFO, enabled by --verbose/-v: the last unmatched "→" names the culprit. This is how the DELETE was isolated from the INSERT during the memory investigation, and it is the fastest way back to a diagnosis if loading breaks again. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Correctness: - pending_deletes() no longer swallows every duckdb.Error as an empty backlog. It guarded against an unbuilt lake, but also hid real failures — reporting "0 rows" would silently disable the safeguard that depends on it. Gate on the records table existing and let other errors surface. - Never demand a backlog below one a rewrite has already failed to reach. The big-resource limit (half of --max-pending-deletes, 50M by default) sits below the ~76M floor observed in production, so it was unreachable by construction and guaranteed a full escalation ladder on every large resource. - Escalate the rewrite threshold within a single trigger instead of across resources. The old state ratcheted down and never recovered, so one org that escalated to 0.01 ran every later resource at the most expensive setting. - Warn before an unbounded mid-sync rewrite, as the compact command already did. Cleanup: - Share the --max-pending-deletes/--verbose option definitions between sync and sync-all rather than duplicating ~20 lines each. - Fold run_rewrite() into run() with a measure argument. - Trim the constant/docstring narrative now that the README covers it. Also: --log writes progress to a file, disables progress bars and keeps the console quiet (cron only mails on failure), and sync logs a line per download — the download bar auto-hides off a TTY, so piping through tee lost it entirely. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- --log claimed a quiet console but did not silence DuckDB's own progress bar: connect() enables it for any TTY, so an interactive run stayed noisy despite the log file. The original test could not catch this because CliRunner is not a TTY, so the new test asserts the setting directly. - compact had no --log, though it is the long-running half of the documented sync -> compact -> publish cron cycle, and its summaries bypassed the helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Mutating the escalation loop's exit condition (<= to <) hung instead of failing: max(MIN, t/2) clamps at exactly MIN, so the comparison never fires and the loop calls compact forever. Replace the while/break with a for over a precomputed ladder, so the escalation is bounded by construction rather than by getting a comparison right. Add direct tests for the ladder, the floor it records, and the disable switch; both a truncated ladder and a dropped floor now fail them. Previously the most intricate code on the branch was only covered incidentally. Downloads were announced twice in a terminal (our line plus the bar's own label). Emit the line only when the bar will be invisible, and give resourcesync.download a `quiet` flag so --log actually silences it — disable=None only auto-hides off a TTY, so --log from a terminal still drew the bar. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Both raise the root logger to INFO, so httpx logged a line per request. On a 300-resource org that buries the progress lines the options exist to surface. Co-Authored-By: Claude Opus 4.8 <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.
The text below was written by Claude. This was a very gnarly problem that I spent days on. During a delete of one record from the lake the memory usage on a 32 GB machine maxed out and started swapping. The solution turned out to be compacting our data in a particular way, on a particular cadence during the synchronization process. I'm adding this to the top just in case future me (or someone else) goes looking for what to do.
DuckLake records deletes as tombstones and only clears them when a data file is
rewritten.
merge_adjacent_filesskips files already at the target size, andducklake_rewrite_data_files— which does clear them — defaults to a 0.95threshold, so podlake never applied any. 1.5B tombstoned rows accumulated
against ~5B live, while
compactreported "nothing to reclaim."That backlog is what made deltas fail. Every DELETE loads the delete history of
each data file it opens, off-pool and not bounded by
memory_limit, so its costtracked the backlog rather than the rows being deleted — a 15k-id delete and a
300k-id delete behaved identically, and Harvard's deltas OOM-killed a 32GB
machine regardless of batch size.
Changes
compactapplies deletes by default (--delete-threshold,--no-rewrite-deletes,--max-files), repeats the expire → merge → cleanupcycle to a fixed point (one pass never fully compacts), and always reports the
pending backlog.
sync/sync-allclear the backlog before each resource, sized to thatresource, escalating the rewrite threshold rather than tolerating a growing
backlog (
--max-pending-deletes).--verboselogs per-statement apply timings — how the DELETE was originallyisolated from the INSERT.
--logwrites progress to a file and keeps theconsole quiet, so a cron run only mails on failure.
sync → compact → publishcycle.Validation
Loaded the full consortium: 13 orgs, ~84.5M records, ~71GB of source MARCXML,
statusat 100%. Stepping the threshold down cleared 93% of the existingbacklog in ~8.5 minutes, and the identical DELETE that had exhausted RAM and
swapped then completed — same ids, same partition, same
memory_limit.Reported upstream as duckdb/ducklake#1371.
Known, deliberately deferred
delete_stateis per-org, but the backlog is table-wide, so each org insync-allrediscovers the same floor.days=0, expiring snapshots — sync now implicitlydiscards rollback points.
🤖 Generated with Claude Code