refactor: implement private transaction and snapshot - #959
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change adds crate-private private transactions for catalog DDL, moves catalog staging into ChangesPrivate transaction and runtime migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant DDLExecution
participant PrivateTransaction
participant CatalogStorage
participant CatalogTables
participant Redo
DDLExecution->>PrivateTransaction: begin private DDL transaction
DDLExecution->>CatalogStorage: stage create or drop metadata
CatalogStorage->>CatalogTables: stage catalog row mutations
CatalogStorage->>Redo: install one transaction-level DDL redo marker
DDLExecution->>PrivateTransaction: commit or rollback catalog DDL
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| Complexity | 5 medium |
🟢 Metrics 63 complexity · 85 duplication
Metric Results Complexity 63 Duplication 85
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Greptile SummaryThe PR introduces strongly owned private transactions for catalog DDL and transaction-free registered snapshots for maintenance reads.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains in the eligible follow-up review scope. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| doradb-storage/src/trx/mod.rs | Adds the strongly attached private-transaction owner, retained checkout lifecycle, private statement settlement, and transaction-level DDL marker installation. |
| doradb-storage/src/trx/readonly.rs | Adds registered private snapshots whose Drop implementation deregisters their STS from the active GC horizon. |
| doradb-storage/src/catalog/storage/ddl.rs | Centralizes CREATE and DROP catalog staging into ordered per-logical-table statement boundaries. |
| doradb-storage/src/catalog/table.rs | Migrates table DDL to private transactions and parks active transaction state before supervised panic retention. |
| doradb-storage/src/catalog/index.rs | Migrates index DDL to private transactions and metadata-driven catalog staging while preserving rollback and panic handling. |
| doradb-storage/src/table/gc.rs | Replaces cleanup transactions with fresh private snapshots for each root-capture attempt and releases registrations on retry, completion, and unwind. |
| doradb-storage/src/session.rs | Refactors mandatory maintenance into stateful execution ownership and exposes private transaction creation only within accepted DDL. |
| doradb-storage/src/recovery/mod.rs | Adapts recovery-side catalog transaction setup to the new transaction-level DDL redo representation. |
Reviews (2): Last reviewed commit: "resolve task" | Re-trigger Greptile
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #959 +/- ##
========================================
Coverage 93.51% 93.51%
========================================
Files 152 154 +2
Lines 131362 131555 +193
========================================
+ Hits 122840 123022 +182
- Misses 8522 8533 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
doradb-storage/src/catalog/storage/ddl.rs (1)
19-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting catalog-row derivation into a helper.
stage_create_tablemixes row derivation with statement staging. Static analysis reports 84 lines and cyclomatic complexity 14. Extract the fourVecbuilders into one private function that returns the derived rows. The staging body then reads as four boundaries plus one redo marker, and the derivation becomes unit-testable without a transaction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doradb-storage/src/catalog/storage/ddl.rs` around lines 19 - 93, Extract the table, column, index, and index-column Vec construction from stage_create_table into a private helper that returns all four derived row collections. Update stage_create_table to call this helper, retain the existing four staging boundaries and redo marker, and keep derivation independent of the transaction so it can be unit-tested separately.Source: Linters/SAST tools
doradb-storage/src/table/mod.rs (1)
1384-1388: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the stale assertion message after the hook rename.
The method and field are now
install_cleanup_after_private_snapshot_hookandcleanup_after_private_snapshot_hook. The assertion still reports "MemIndex cleanup transaction-start hook already installed", which names the removed transaction-start concept.♻️ Proposed fix
assert!( old.is_none(), - "MemIndex cleanup transaction-start hook already installed" + "MemIndex cleanup private-snapshot hook already installed" );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doradb-storage/src/table/mod.rs` around lines 1384 - 1388, Update the assertion message in install_cleanup_after_private_snapshot_hook to describe the cleanup-after-private-snapshot hook, replacing the stale MemIndex transaction-start wording while preserving the existing assertion behavior.doradb-storage/src/trx/stmt.rs (1)
340-353: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
Statement::newinStmtState::statement.
StmtState::statementat lines 264-280 builds the sameStatementliteral with the same defaults. CallStatement::newthere so the construction of a callback-facing statement stays in one place.♻️ Proposed refactor
let (inner, attachment) = checkout.inner_and_attachment_mut(); - Statement { - inner, - attachment, - effects, - disable_dml_validation: false, - } + Statement::new(inner, attachment, effects)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doradb-storage/src/trx/stmt.rs` around lines 340 - 353, Update StmtState::statement to construct the callback-facing statement by calling Statement::new with the existing inner, attachment, and effects references, instead of duplicating the Statement literal and its defaults. Leave the surrounding statement flow unchanged.doradb-storage/src/table/gc.rs (1)
268-286: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBind the registered snapshot once instead of re-reading it twice.
Lines 269-275 and 284-286 read
execution.active_snapshotback with two unreachablepanic!arms right after the assignment. Register into a local, readsts()from it, then move it intoexecution.active_snapshot, and borrow it once for the root capture. This removes both defensive panics without changing behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doradb-storage/src/table/gc.rs` around lines 268 - 286, Update the cleanup loop to store the result of trx_sys.register_private_snapshot() in a local variable, read its sts() before moving it into execution.active_snapshot, and then borrow execution.active_snapshot once for root capture. Remove both unwrap_or_else panic paths while preserving the existing snapshot registration and scanning behavior.doradb-storage/src/trx/mod.rs (1)
3551-3561: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the existing active-list length accessor.
active_sts_countrecomputes the live count asactive.len() - active.deleted.len().ActiveStsListalready exposeslen(), which other code uses (for example indoradb-storage/src/trx/purge.rstests). The manual subtraction duplicates that logic and panics on unsigned underflow if the two collections ever diverge.♻️ Proposed refactor
pub(crate) fn active_sts_count(trx_sys: &sys::TransactionSystem) -> usize { trx_sys .gc_buckets .iter() - .map(|bucket| { - let active_sts = bucket.active_sts_list.lock(); - active_sts.active.len() - active_sts.deleted.len() - }) + .map(|bucket| bucket.active_sts_list.lock().len()) .sum() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doradb-storage/src/trx/mod.rs` around lines 3551 - 3561, Update active_sts_count to call ActiveStsList::len() for each locked bucket instead of manually subtracting active.deleted.len() from active.len(), preserving the existing sum of live snapshot counts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@doradb-storage/src/table/gc.rs`:
- Around line 287-295: Update the health-check error path around
execute_mem_index_cleanup_inner and execution.phase so an ensure_healthy failure
releases execution.active_snapshot and deregisters the associated STS before
returning. Set the execution state consistently with the existing explicit
release path, while preserving the current RuntimeOrFatalError conversion and
normal-path behavior.
---
Nitpick comments:
In `@doradb-storage/src/catalog/storage/ddl.rs`:
- Around line 19-93: Extract the table, column, index, and index-column Vec
construction from stage_create_table into a private helper that returns all four
derived row collections. Update stage_create_table to call this helper, retain
the existing four staging boundaries and redo marker, and keep derivation
independent of the transaction so it can be unit-tested separately.
In `@doradb-storage/src/table/gc.rs`:
- Around line 268-286: Update the cleanup loop to store the result of
trx_sys.register_private_snapshot() in a local variable, read its sts() before
moving it into execution.active_snapshot, and then borrow
execution.active_snapshot once for root capture. Remove both unwrap_or_else
panic paths while preserving the existing snapshot registration and scanning
behavior.
In `@doradb-storage/src/table/mod.rs`:
- Around line 1384-1388: Update the assertion message in
install_cleanup_after_private_snapshot_hook to describe the
cleanup-after-private-snapshot hook, replacing the stale MemIndex
transaction-start wording while preserving the existing assertion behavior.
In `@doradb-storage/src/trx/mod.rs`:
- Around line 3551-3561: Update active_sts_count to call ActiveStsList::len()
for each locked bucket instead of manually subtracting active.deleted.len() from
active.len(), preserving the existing sum of live snapshot counts.
In `@doradb-storage/src/trx/stmt.rs`:
- Around line 340-353: Update StmtState::statement to construct the
callback-facing statement by calling Statement::new with the existing inner,
attachment, and effects references, instead of duplicating the Statement literal
and its defaults. Leave the surrounding statement flow unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: dea478d8-98de-4019-8376-f86680864733
⛔ Files ignored due to path filters (5)
docs/garbage-collect.mdis excluded by none and included by nonedocs/lock-system.mdis excluded by none and included by nonedocs/tasks/000262-private-transactions.mdis excluded by none and included by nonedocs/transaction-system.mdis excluded by none and included by nonedocs/unsafe-usage-baseline.mdis excluded by none and included by none
📒 Files selected for processing (21)
doradb-storage/src/catalog/checkpoint.rsdoradb-storage/src/catalog/index.rsdoradb-storage/src/catalog/storage/columns.rsdoradb-storage/src/catalog/storage/ddl.rsdoradb-storage/src/catalog/storage/indexes.rsdoradb-storage/src/catalog/storage/mod.rsdoradb-storage/src/catalog/storage/tables.rsdoradb-storage/src/catalog/table.rsdoradb-storage/src/recovery/mod.rsdoradb-storage/src/session.rsdoradb-storage/src/table/access.rsdoradb-storage/src/table/gc.rsdoradb-storage/src/table/mod.rsdoradb-storage/src/table/persistence.rsdoradb-storage/src/table/storage.rsdoradb-storage/src/trx/mod.rsdoradb-storage/src/trx/purge.rsdoradb-storage/src/trx/readonly.rsdoradb-storage/src/trx/retention.rsdoradb-storage/src/trx/stmt.rsdoradb-storage/src/trx/sys.rs
1 participant
Closes #958
Summary by CodeRabbit
New Features
Bug Fixes