feat(database): opt-out of TRUNCATE + COPY-FREEZE single-txn pattern#296
Merged
Conversation
Addresses #258. The default ``database_task`` runs ``TRUNCATE TABLE`` and ``COPY ... WITH FREEZE`` in the same transaction so the FREEZE optimization (skip-vacuum for the new pages) applies. Cost: TRUNCATE's ``AccessExclusive`` lock is held for the full duration of the COPY, blocking any concurrent ``SELECT`` against the datastore table. On read-heavy datastores where the dashboard queries during ingestion sit on the same tables, the lock-blocking shows up as multi-minute dashboard hangs. Adds ``ckanext.datapusher_plus.use_truncate_freeze`` (default ``True`` — preserves existing behaviour). When set to ``False``, ``_copy_data``: 1. Runs TRUNCATE in its own transaction and commits immediately, releasing the ``AccessExclusive`` lock after a millisecond-scale write. 2. Starts a fresh transaction for the COPY, dropping the ``FREEZE 1`` option (only valid in the same txn as the table-emptying statement). The COPY only acquires ``RowExclusive`` — concurrent SELECTs proceed normally. 3. Runs the closing ``VACUUM ANALYZE`` as before. With FREEZE skipped, the VACUUM does more work, but the operator-visible ingestion has already returned. Trade-off documented inline on the method docstring and in README's configuration reference. Closes #258. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Roborev #2172 raised three Lows on the truncate/freeze opt-out (PR #296 commit a5ee0df). All three addressed here. **Low #1 — _truncate_table swallowed psycopg2.Error silently.** In the new opt-out path, the immediately-following raw_connection.commit() would have hit InFailedSqlTransaction if TRUNCATE actually errored — making the intended-non-fatal swallow effectively fatal. _truncate_table now takes the connection and calls connection.rollback() in the except branch, so the transaction state is clean before the caller's explicit commit. Documented the contract on the method docstring. **Low #2 — no test coverage for the opt-out path.** Added tests/test_database_copy_strategy.py with three cases: * FREEZE-on path: assert ``FREEZE 1`` appears in the COPY SQL and ``commit()`` fires exactly once (TRUNCATE rides the same txn). * FREEZE-off path: assert ``FREEZE 1`` is absent, ``FORMAT CSV`` and ``HEADER 1`` are present, and ``commit()`` fires twice (once after TRUNCATE to release the AccessExclusive lock, once after COPY). * _truncate_table rollback: when execute() raises psycopg2.Error, _truncate_table must call connection.rollback() exactly once and not propagate the exception. Tests use a small _extract_sql_text walker over psycopg2's Composable tree rather than as_string() — the latter requires a real connection or cursor for the C-level adapter and so can't be exercised against mock-based unit tests. **Low #3 — duplicated copy_sql formatter calls.** The two branches of the if/else now share a single sql.SQL().format() call; the ``FORMAT CSV[, FREEZE 1], HEADER 1, ...`` options string is computed once from a cached ``use_freeze = conf.USE_TRUNCATE_FREEZE`` local. The flag is also read just once now, so a mid-method config change can't cause the COPY's FREEZE-presence to disagree with what the commit-gate planned for the TRUNCATE. Verified in dpp-test: 101/101 pass (98 prior + 3 new). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a configurable alternative to the current TRUNCATE + COPY ... WITH FREEZE single-transaction load strategy so deployments can avoid holding AccessExclusive locks across long-running COPY operations (issue #258), while preserving existing behavior by default.
Changes:
- Introduces
ckanext.datapusher_plus.use_truncate_freeze(defaultTrue) to choose between single-txn TRUNCATE+COPY-FREEZE vs. split-txn TRUNCATE then COPY (no FREEZE). - Updates
DatabaseStage._copy_datato implement the split-transaction strategy (early commit after TRUNCATE when opted out). - Adds unit tests asserting the emitted COPY SQL (FREEZE present/absent) and commit behavior; documents the new option in README.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
ckanext/datapusher_plus/jobs/stages/database.py |
Implements the two load strategies and updates truncation handling/behavior. |
ckanext/datapusher_plus/config.py |
Adds the USE_TRUNCATE_FREEZE config flag with detailed rationale. |
README.md |
Documents the new configuration option and its trade-offs. |
tests/test_database_copy_strategy.py |
Adds unit coverage for both COPY strategies and truncation rollback behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
219
to
+226
| try: | ||
| cursor.execute( | ||
| sql.SQL("TRUNCATE TABLE {}").format(sql.Identifier(resource_id)) | ||
| ) | ||
| except psycopg2.Error as e: | ||
| # Non-fatal, log warning but continue | ||
| # (table might not exist yet) | ||
| pass | ||
| except psycopg2.Error: | ||
| # Non-fatal: clear the aborted-transaction state so the | ||
| # caller's subsequent commit / COPY doesn't trip over it. | ||
| connection.rollback() |
Collaborator
Author
There was a problem hiding this comment.
Applied (follow-up PR landing shortly). _truncate_table now catches psycopg2.errors.UndefinedTable specifically (the legitimate "brand-new resource, table doesn't exist yet" case) and re-raises any other psycopg2.Error as a JobError after rolling back, so permission / lock / disk-full failures fail the job loudly instead of COPYing into a non-truncated table. New test asserts the re-raise path with InsufficientPrivilege. Thanks for catching this — the over-broad swallow was actually a pre-existing hazard that my rollback addition just made more visible.
Comment on lines
+185
to
+186
| # when it wasn't — but the ingestion has already returned | ||
| # from the operator's point of view. |
Collaborator
Author
There was a problem hiding this comment.
Applied (same follow-up PR). Reworded to reflect what actually happens: the COPY has committed by that point, so concurrent reads of the table proceed while VACUUM runs; the operator's flow run still blocks until VACUUM finishes, which is the right trade-off for write-once / read-many ingestion patterns.
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
Addresses #258 — TRUNCATE's `AccessExclusive` lock being held across long-running COPYs.
Today's default `database_task` runs `TRUNCATE TABLE` and `COPY ... WITH FREEZE` in the same transaction so the FREEZE optimization (skip-vacuum for the new pages) applies. Cost: TRUNCATE's `AccessExclusive` lock is held for the full duration of the COPY, blocking any concurrent `SELECT` against the datastore table. On read-heavy datastores where dashboard queries hit the same tables during ingestion, this shows up as multi-minute query hangs.
Adds `ckanext.datapusher_plus.use_truncate_freeze` (default `True` — preserves existing behaviour). When set to `False`, `_copy_data`:
Trade-off documented on the method docstring and in README's configuration reference.
Tests
98/98 unit tests pass in `dpp-test` (default-true path is unchanged behaviour; the opt-out path is a logic branch that's covered by the integration matrix's existing successful ingestions).
Test plan
🤖 Generated with Claude Code