ADFA-5141: Pin doc DB page_size to 2048 in vacuum_database - #25
ADFA-5141: Pin doc DB page_size to 2048 in vacuum_database#25davidschachterADFA wants to merge 8 commits into
Conversation
The 1024-byte page_size (SQLite's old default) carries ~2x the per-page header overhead of 2048 on the real ~300MB docdb, with no tradeoff versus larger sizes tested. vacuum_database() already rewrites the whole file on every delete-bearing mutation, so setting PRAGMA page_size before that VACUUM migrates the file for free on the next regeneration rather than as a one-off. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Trigger the migration on pure-insert paths too (import_content_files, import_csv_rows), not just delete/overwrite-gated ones, via a new _page_size_migration_pending check. - Work around WAL journal mode silently preventing PRAGMA page_size from taking effect on VACUUM. - Pin page_size on the release pipeline's DocumentationDatabase at creation time, so the actually-shipped DB gets the fix for free instead of only a maintainer's locally vacuumed copy. - Raise fetch_content_for_path's busy timeout so a concurrent VACUUM can't surface real content as a false 404. - Correct vacuum_database's docstring (VACUUM is never a no-op) and align its connection handling with the repo's with-block convention. - Update README's "no migrations" claim and add test coverage for the real 1024->2048 migration, WAL mode, and both pure-insert paths. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Addressed a
The All 181 docdb-studio tests and 7 DocumentationDatabase tests pass. |
- vacuum_database: wrap the journal_mode switch/VACUUM/restore in try/finally so a lock error mid-VACUUM can't leave the DB stuck in DELETE journal mode forever; also dedupe the was-WAL check. - _page_size_migration_pending: match fetch_content_for_path's 30s busy timeout, and cache confirmed-migrated paths so it stops reopening a connection on every import call once the one-time migration is done. - Add a cross-file test asserting scripts/DocumentationDatabase.py and docdb-studio/docdb_studio.py agree on SQLITE_PAGE_SIZE_BYTES, since the two tools are separately deployed with no shared import. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The in-place VACUUM + journal_mode round-trip required exclusive access to db_path: SQLite refuses to switch a WAL-mode database away from WAL while ANY other connection has it open. Empirically reproduced this as a deterministic deadlock in import_csv_rows/import_content_files et al, and confirmed it goes deeper than "close the caller's own connection" -- an already-returned function's unclosed `with sqlite3.connect(...) as conn:` block can keep the file locked well past its own return, and that convention is used 30+ times in this file. A live UI connection left open elsewhere (e.g. a data browser) would hit the same deadlock. vacuum_database now rewrites into a temp file via VACUUM INTO, which only needs a read snapshot of the source, and atomically swaps it into place with os.replace -- this works regardless of what else has db_path open. journal_mode=WAL is reapplied to the new file's final path (VACUUM INTO always produces a plain rollback-journal file), and stale sidecars from the replaced file are cleaned up. Also, from the second review pass: - vacuum_database's connect uses the 30s timeout matching the other ADFA-5141 call sites. - delete_tooltips_bulk now also triggers on _page_size_migration_pending, not just `deleted` -- it had the same gap the import paths were already fixed for. - _page_size_confirmed is now lock-guarded (imports can run migration checks from worker threads). - Callers close their own connections before any vacuum-triggering call, matching the (no longer strictly required, but still good practice) discipline established while chasing the deadlock down. New tests cover: the WAL deadlock is gone even with an unrelated open connection AND an unclosed caller-style connection present at once, the original file is left untouched if VACUUM INTO fails partway (temp file cleaned up, no partial swap), and the migration-pending gate on each previously-gapped call path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Was able to run the tests in this PR and verify that the page size got updated. However Claude pointed out that mkstemp() creates all files with 0600 permissions so 0600 would overwrite whatever the permissions were beforehand: QA result: PR #25 works, but with one bug found Migration confirmed working: ~/documentation.db page_size: 1024 → 2048 ✅ Bug found during QA: vacuum_database() silently drops the database file's permissions from 644 (owner+group+other readable) to 600 (owner-only) every time it runs. It rewrites via tempfile.mkstemp() (which always creates files 0600) and then os.replace()s that temp file into place, never restoring the original file's mode. On your machine this hit — ~/documentation.db went from -rw-r--r-- to -rw------- immediately after the migration ran. I've restored it to 644 by hand, but the underlying code will re-break it on every future vacuum. This isn't covered by the existing test suite (no test checks file mode after vacuum_database). |
vacuum_and_pin_page_size (commit b203500) mirrored docdb-studio.py's original vacuum_database(): in-place VACUUM + a journal_mode round-trip, which requires exclusive access to db_path. SQLite refuses to switch a WAL-mode database away from WAL while ANY other connection has it open -- even one from a function that has already returned, since Python's `with sqlite3.connect(...) as conn:` does not close conn on exit. Empirically reproduced and fixed the identical bug in docdb-studio.py's vacuum_database (PR #25); this mirrors that fix here since this pipeline's own VACUUM is the one actually run against the live documentation.db. Rewritten on VACUUM INTO: rebuild into a temp file next to db_path (read-only snapshot of the source, no exclusive access needed), then atomically swap it into place with os.replace. journal_mode=WAL is reapplied to the new file's final path (VACUUM INTO always produces a plain rollback-journal file), and stale sidecars from the replaced file are cleaned up. Two new tests: the fix succeeds with both an unrelated open connection and an unclosed caller-style connection present at once (the actual scenario the old design was fragile against), and the original file is left untouched if VACUUM INTO fails partway (temp file cleaned up, no partial swap). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Summary of the last two review roundsRound 2 findings (all fixed, commit 1018a57):
Round 3 (commit 7bed792) — a deeper problem found while testing round 2's fix: While writing a regression test for "the caller's own connection must be closed before Rewrote Also applied the identical fix to 190/190 tests passing on this branch. |
tempfile.mkstemp() always creates its file mode 0600 regardless of the original's mode or the process umask. The VACUUM INTO rewrite swaps that temp file into db_path's place via os.replace, which never restored the original permissions -- alexmmiller's QA on real hardware caught this silently dropping documentation.db from 644 to 600 on every vacuum. Capture db_path's mode before the rewrite and os.chmod it back after the swap. New test confirms a 644 file stays 644 across vacuum_database (and fails against the pre-fix code, dropping to 600). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rewrite - Escape embedded single quotes in the VACUUM INTO target path. It's a SQL string literal, not a bindable parameter, so a db_path whose parent directory contains a quote (a real user directory name like "David's Docs") broke the statement outright -- confirmed empirically. - Serialize vacuum_database via a module-level lock. VACUUM INTO takes a read snapshot; without this, two concurrent invocations on the same db_path could race, with the second's os.replace silently discarding a write the first's snapshot missed. This only serializes vacuum_database against itself -- see its docstring for the residual gap against a fully independent concurrent writer. - Close the journal_mode-read connection explicitly instead of relying on it being reassigned by the next `with` block, matching every other connection touched by this fix. - insert_tooltip/update_tooltip/add_tooltip_button/update_tooltip_button never called vacuum_database at all (no delete, so the DB-hygiene gate never fired), so a docdb edited only through those single-row paths never migrated to SQLITE_PAGE_SIZE_BYTES. Added the same _page_size_migration_pending gate used elsewhere -- cheap once cached, so this only costs a real VACUUM the one time it's needed. New tests: quote-in-path handling, concurrent vacuum_database calls don't raise or corrupt the file, and each of the four previously-gapped single-row mutation functions now migrates page_size. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Cleaner than manually escaping embedded single quotes: VACUUM INTO's target accepts a bound parameter (already used by populate_db.py's backup_database for the same reason), so this sidesteps SQL string-literal escaping entirely rather than hand-rolling it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for catching this @alexmmiller — confirmed and fixed in A follow-up self-review turned up a few more real issues in the same rewrite, all fixed and pushed:
197/197 tests passing. |
- vacuum_database: chmod the temp file to the original permissions before os.replace, not after -- fixing it up afterward left a real (if brief) window where db_path was visible at mkstemp's 0600, and left permissions permanently wrong if the chmod itself failed. - vacuum_database: explicitly close the VACUUM INTO and WAL-reapply connections, and give the WAL-reapply connection the same 30s timeout as its siblings in the same function -- empirically reproduced a ResourceWarning: unclosed database for the two that were missed, and the untimed connection could abort the function after the rewrite/permissions/sidecar-cleanup already succeeded, leaving _page_size_confirmed never updated. - delete_tooltips_bulk: the empty-tooltip_ids early return skipped the migration-pending check entirely, unlike its non-empty "nothing matched" case which was already covered. - get_category_name/update_last_change: same 30s timeout as fetch_content_for_path, for the same reason -- they run right after replace_tooltip_buttons's unconditional vacuum in the tooltip-save flow. - Introduced _vacuum_if_dirty_or_pending() as a single choke point for the "vacuum after a mutation, or to migrate page_size on a pure insert" gate, replacing 6 near-identical copies of it (the 7th, import_content_files, keeps its own inline version since it wraps the gate with progress-callback reporting). A future new mutating helper can no longer add itself without also getting the migration check for free. New tests: the empty-list migration gap, plus verified (and reverted to confirm) that each fix here catches a real regression against the pre-fix code. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same findings as the mirrored docdb-studio.py fix's third self-review: - chmod the temp file to the original permissions before os.replace, not after -- fixing it up afterward left a real window where db_path was visible at mkstemp's 0600, and left permissions permanently wrong if the chmod itself failed. - Explicitly close the VACUUM INTO and WAL-reapply connections, and give the WAL-reapply connection the same 30s timeout as its siblings in the same function. 19/19 local tests pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Third self-review pass fixed: chmod-before-replace ordering (a failed chmod previously left permissions permanently wrong), two unclosed connections in vacuum_database (empirically confirmed via ResourceWarning), a missing 30s timeout on the WAL-reapply connection, delete_tooltips_bulk's empty-list early-return skipping the migration check, timeout consistency on get_category_name/update_last_change, and consolidated 6 near-identical migration-gate call sites into one helper. 198/198 tests passing. |
|
Declining this per ADFA-5141: benchmarking showed essentially the same performance for page_size 1024 vs 2048, and a negligible size difference before compression (likely even less after ADFA-5153's dictionary compression on the Content table). Adds complexity without benefit — not pursuing this further. See ADFA-5141 for the full analysis and benchmark data. |
2 participants
Summary
page_size=1024(the current docdb setting) carries ~2x the per-page header overhead of 2048 on the real ~300MB docdb, with no size tradeoff versus other sizes tested (see ADFA-5124 comments' page-size sweep — 2048 was smallest of the sizes tried).vacuum_database()already rewrites the whole DB file on every delete-bearing mutation (project DB-hygiene policy). SettingPRAGMA page_size=2048immediately before thatVACUUMmigrates the file to the new page size for free on the next regeneration, rather than requiring a one-off migration script.PAGE_SIZEUI-pagination constant toUI_PAGE_SIZEto avoid confusion with the newSQLITE_PAGE_SIZE_BYTESconstant.Test plan
pytest tests/— 178/178 pass, including newtest_vacuum_database_sets_target_page_sizeasserting the pragma sticks afterVACUUM.Jira: ADFA-5141
🤖 Generated with Claude Code