Repository navigation
fix(duckdb_pool): apply MINILAKE_DUCKDB_MEMORY_LIMIT to every DuckDB connection - #68
Merged
Merged
Conversation
…connection The setting was parsed but never passed to duckdb.connect, so every database ran with DuckDB's default of 80% of host RAM. Pass memory_limit through _connect_config, which both the Unity Catalog and warehouse connections use. An invalid value now fails at startup instead of being ignored. Closes dmux#51
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.
Closes #51
Summary
MINILAKE_DUCKDB_MEMORY_LIMITis documented as the way to cap DuckDB's memory (default4GB),but it was parsed into
settings.duckdb_memory_limitand never passed to any connection. EveryDuckDB database therefore ran with DuckDB's own default of 80% of host RAM, so users relying on
the setting to keep minilake inside a CI runner or a memory-limited container had no protection.
Before / after
Root cause
DuckDBPool._connect_config()builds theconfig=for bothduckdb.connectcalls (the sharedUnity Catalog connection and each warehouse connection), and it only ever carried the extension
directory settings, so
memory_limitnever reached DuckDB.What changed
DuckDBPool.__init__takes an optionalmemory_limit, falling back tosettings.duckdb_memory_limit, mirroring the existingextension_dirparameter._connect_config()always includesmemory_limit. Both connection paths already go through it,so one change covers both.
lots) makesduckdb.connectraise. The UC connection is opened duringstartup, so a bad setting now stops the server instead of being silently ignored. No separate
validation was added, so DuckDB's own parser stays the single source of truth.
docs/configuration.mdno longer describes the setting as a no-op, and notes that the limitapplies per database (the shared UC database and each warehouse database each get their own).
Passing the limit through
config=was chosen overSET memory_limitafter connecting: it isatomic (no window where the conn 8000 ection runs unlimited) and reuses the existing helper.
Testing
Full suite via
docker-compose.test.yml, image rebuilt:ruff check,ruff format --checkand pre-commit hooks cleanNew tests in
tests/test_duckdb_memory_limit.py:test_statement_execution_uses_configured_memory_limit: via the real databricks-sdk, theengine's
current_setting('memory_limit')matches the configured value.test_every_pool_connection_gets_the_memory_limit: both the UC connection and a warehouseconnection apply a custom limit (statements run on the UC connection, so the SDK test alone
would not cover warehouse connections).
test_invalid_memory_limit_fails_loudly: a malformed limit raises on connect.Risk / rollout
The documented
4GBdefault now actually takes effect, which lowers the effective cap on hostswith more than ~5 GB of RAM. DuckDB spills large operators to disk, so most heavy queries slow
down rather than fail, but a query needing more than 4GB of non-spillable memory will now raise
an out-of-memory error where it previously succeeded. Raising
MINILAKE_DUCKDB_MEMORY_LIMITrestores the old headroom. Probably worth a line in the release notes.