Skip to content

feat(config): let the engine control SystemConfig defaults - #77

Open
adsharma wants to merge 2 commits into
mainfrom
engine-controlled-defaults
Open

adsharma wants to merge 2 commits into
mainfrom
engine-controlled-defaults

Conversation

@adsharma

Copy link
Copy Markdown
Contributor

Follow-up to #76 (which simply flipped the default): instead of pinning a copy of every engine default in this binding, all engine settings now default to None, and only explicitly set values are forwarded to either backend.

  • database.py: settings params default to None; new _engine_kwargs() forwards only set values to both backends.
  • _lbug_capi.py: params default to None; applies only set values onto lbug_default_system_config().
  • py_database.cpp/py_database.h: constructor takes std::optional params (default py::none()) applied onto SystemConfig().

Unset settings now follow engine-side default changes automatically (e.g. LadybugDB/ladybug#1435's enable_multi_writes flip needs no binding change). Explicitly passed values behave exactly as before, so existing code passing values is unaffected.

BREAKING CHANGE: settings parameters changed from concrete defaults to None = "engine default". Code that only reads db.enable_multi_writes etc. may now see None instead of the old hardcoded value.

Stop pinning a copy of every engine default in this binding. All
engine settings now default to None, and only explicitly set values
are forwarded to either backend (capi starts from
lbug_default_system_config(), pybind from SystemConfig()). Unset
settings now follow engine-side default changes automatically.

BREAKING CHANGE: Database() settings parameters changed from concrete
defaults (e.g. enable_multi_writes=False) to None meaning "engine
default"; explicitly passed values behave as before.
The binding no longer pins throw_on_wal_replay_failure=True; unset
settings follow the engine default (false: silently ignore). This test
verifies that a checksum mismatch raises, so it must now opt into
throwing explicitly.
@adsharma

Copy link
Copy Markdown
Contributor Author

CI failure addressed: test_database_enable_checksums_config relied on the binding pinning throw_on_wal_replay_failure=True. With engine-controlled defaults the second open silently ignored the checksum mismatch instead of raising. Fixed by opting into throwing explicitly in the test (its intent is checksum-mismatch detection, not the throw default). Verified locally on the C-API backend: unfixed test fails with the same DID NOT RAISE, fixed test passes, and the full test_database.py module (11 tests) is green.

@adsharma

Copy link
Copy Markdown
Contributor Author

There is also a question of not changing the defaults on our largest user base (python).

Should the engine also switch to throw_on_wal_replay_failure=True? @zahariash @aikins01

@adsharma adsharma changed the title feat!: let the engine control SystemConfig defaults feat(config): let the engine control SystemConfig defaults Oct 27, 2026
@zahariash

Copy link
Copy Markdown
Contributor

I'd keep the engine on false, but narrow it before Python follows.

true turns every crash during a commit into a database that won't open until the user finds this flag (LadybugDB/ladybug#714, reported from Python). Killing a writer at each of 4,231 file syscalls on main left 369 such databases, and false opened all of them correctly. LadybugDB/ladybug#1380 and LadybugDB/ladybug#1400 rely on it too.

The problem is that false drops more than a torn tail: dryReplay catches every exception and truncates the WAL there. On 0.22.0, with three commits in the WAL:

WAL damage true false
torn tail refuses commits 1 and 2
one byte flipped in commit 2 refuses commit 1; commit 3 lost for good
one EIO while reading commit 3 refuses commits 1 and 2; commit 3 lost for good
other enable_checksums (LadybugDB/ladybug#1357) refuses WAL deleted

DuckDB's rule fits our WAL framing: drop only a record that runs past EOF, or a bad last record with nothing valid after it, and throw on everything else whatever the flag. I can open that as a draft under LadybugDB/ladybug#1376. After it, all Python users notice from the new WAL default is that a crash-torn database opens.

Until then, LadybugDB/ladybug#1358 should land first: the test edit here and the test_database_enable_checksums failure on LadybugDB/ladybug-rust#45 are both LadybugDB/ladybug#1357.

@aikins01

Copy link
Copy Markdown

Agree on keeping the engine on false.

On order:

  1. fix(wal): throw on a checksum-setting mismatch instead of dropping the WAL ladybug#1358 first. I went through it and it looks right to me.
  2. Then this PR rather than fix(database): open past a torn WAL tail by default, as the engine does #75. Both switch Python to the lenient default in the same lines, and this one also stops the binding from copying the other engine defaults. Once #1358 is in the engine build CI uses, the throw_on_wal_replay_failure=True edit in test_database_enable_checksums_config can go, so the test checks the new default. fix(database): open past a torn WAL tail by default, as the engine does #75's torn-tail test and its throw_on_wal_replay_failure docstring are worth bringing over too; the docstring here still says a replay failure is silently ignored, not that later commits are dropped.
  3. Then the stricter dryReplay under Tracking: commit, checkpoint and WAL recovery fixes found by crash and stress testing ladybug#1376. I wouldn't hold Python back for it: when a Python user hits a refusal today, the way out is deleting the WAL by hand (WAL becomes unreplayable after process termination — wal_record.cpp:76 UNREACHABLE_CODE, checksum failure, and invalid-WAL-record variants (repeats ~23x in production on 0.16.0) ladybug#1388 did that about 25 times), which loses more than truncating at the first bad record.

The BREAKING CHANGE note covers reads returning None. It should also say that a database with a torn WAL now opens instead of throwing.

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.

3 participants