Let a deployment bound what DuckDB holds - #977
Merged
Merged
Conversation
DuckDB reads the cgroup, so a container gets a memory_limit sized to its own cap rather than to the host. That limit bounds DuckDB's buffers, not the process, and the default takes about 80% of the cap for them: over ORD the index build holds up to 2.8 GB outside that accounting, so an 8 GiB container running the default 6.3 GiB limit is killed 85s in, while the same build under a 6500MiB limit and a 12 GiB cap finishes at 9.3 GB resident. Corpus took no configuration, so a caller could not set the limit without reaching into the connection. It takes memory_limit now, and warns at open when a cap it can read leaves less headroom than a build wants, since reaching the cap arrives as neither an exception nor a log line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A cap can be stated at any level between the process's own cgroup and
the mount root, and reading only the root misses it wherever the process
sits below one. Under --cgroupns=host the root states no cap at all, so
the warning stayed silent in exactly the deployment that needed it:
/proc/self/cgroup: 0::/docker/afe7dc23...
cap : 8.0 GB
root alone : None
The path comes from /proc/self/cgroup now, matched by controller name so
a v1 line is not read against the v2 mount, and every level from there up
is consulted. The smallest cap wins, since that is the one the process
reaches first.
Co-Authored-By: Claude Opus 5 (1M context) <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
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.
Summary
Corpuscreated its DuckDB connection with no configuration, so a caller could not setmemory_limitwithout reaching into_connection. In a container that is not a small gap: DuckDB reads the cgroup, so it sizes its default limit from the container's own cap rather than the host's — but that limit bounds DuckDB's buffers, not the process, and the default claims about 80% of the cap for them. The occurrence index build holds up to 2.8 GB beside that accounting, so the default is over the cap by construction, and reaching a cap is a kill: no exception, no log line, exit 137.Measured in a container over the full corpus (2,428,291 reactions), calling
check_index():memory_limitOutOfMemoryExceptionat 195s, peak RSS 3.91 GiBOutOfMemoryExceptionat 194s, peak RSS 8.65 GiBThe catchable exception
check_index()documents only arrives when the limit leaves room; the docstring promised it unconditionally.Changes
Corpus(..., memory_limit=...)passes the setting toduckdb.connect._warn_when_the_cap_leaves_no_headroomreads the cgroup at open (v2, then v1, treatingmaxand the top-of-range sentinel as no cap) and warns when it leaves less than 4 GB above the limit.check_indexand the README's deployment section say what a cap has to leave, and the floor reads 5–6.5 GB rather than 5 GB — 5 GiB raised where 6500MiB finished.Testing
uv run pytest -n auto— 1189 passed, 2 skipped.Eleven new tests cover the setting reaching DuckDB, the size parser,
maxand the v1 sentinel reading as no cap, and the warning firing on a cap with no headroom while staying quiet on one with headroom and on a machine with no cgroup to read.End to end in an 8 GiB container, the configuration that was killed above:
Notes
The default is unchanged: no
memory_limitmeans DuckDB decides, as before. The warning is the only new behavior on that path, and it fires only where a cgroup cap is readable.Wall-clock figures above are inflated — the container read the artifacts and spilled through a bind mount. Resident sizes, which are what the change is about, are unaffected.
🤖 Generated with Claude Code
Greptile Summary
The PR lets callers configure DuckDB's memory limit and adds deployment guidance for budgeting memory under container caps.
memory_limitthroughCorpusto DuckDB.Confidence Score: 5/5
The PR appears safe to merge because no blocking failure remains.
No blocking failure remains.
Important Files Changed
Reviews (2): Last reviewed commit: "Read the cgroup this process is in, not ..." | Re-trigger Greptile