refactor: implement runtime-owned table DDL - #925
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCREATE TABLE and DROP TABLE now run as prepared mandatory-runtime operations. The change adds scoped lock ownership, prepared catalog-write authority, lifecycle-aware transaction handling, phase-specific cleanup, panic supervision, and tests for cancellation, abandonment, poisoning, and shutdown. ChangesTable DDL runtime migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Session
participant MandatoryRuntime
participant CatalogTransaction
participant TableRuntime
Session->>MandatoryRuntime: submit accepted CREATE or DROP
MandatoryRuntime->>CatalogTransaction: stage and commit catalog DDL
CatalogTransaction-->>MandatoryRuntime: return commit result
MandatoryRuntime->>TableRuntime: install or retain runtime state
MandatoryRuntime-->>Session: publish completion or retained failure
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
|
| Metric | Results |
|---|---|
| Complexity | 7 |
| Duplication | 0 |
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 moves accepted CREATE TABLE and DROP TABLE execution onto the engine-owned mandatory runtime while retaining cancellable validation and lock preparation on the caller.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains within the eligible follow-up review scope.
|
| Filename | Overview |
|---|---|
| doradb-storage/src/catalog/table.rs | Refactors CREATE and DROP into prepared and accepted execution carriers with explicit progress, compensation, panic, and retention handling. |
| doradb-storage/src/session.rs | Adds caller-side DDL preparation, owned lock scopes, mandatory-operation transitions, and terminal settlement. |
| doradb-storage/src/runtime/mandatory.rs | Updates mandatory execution supervision and completion ownership for accepted table DDL. |
| doradb-storage/src/trx/mod.rs | Extends mandatory operations to own nested private transactions and retain transaction state after supervised panics. |
| doradb-storage/src/trx/stmt.rs | Adds prepared catalog-write authority and panic-safe settlement of catalog statements. |
| doradb-storage/src/trx/sys.rs | Integrates nested mandatory transactions with transaction-system completion and cleanup paths. |
| doradb-storage/src/lock/mod.rs | Removes the superseded foreground CREATE lock helper in favor of operation-owned prepared lock scopes. |
| doradb-storage/src/completion.rs | Promotes exclusive completion consumption APIs from reserved code to production mandatory-runtime use. |
Reviews (2): Last reviewed commit: "fix issues and resolve task" | Re-trigger Greptile
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #925 +/- ##
==========================================
+ Coverage 93.27% 93.32% +0.04%
==========================================
Files 148 148
Lines 123127 123953 +826
==========================================
+ Hits 114845 115677 +832
+ Misses 8282 8276 -6 ☔ 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: 3
🧹 Nitpick comments (6)
doradb-storage/src/trx/mod.rs (2)
1836-1840: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winValidate the state inside
publish_mandatory_terminal.
publish_mandatory_terminalwritesTerminalunconditionally. The validation lives inassert_mandatory_finish_ready, which a caller must invoke separately.MandatoryOperationGuard::finishindoradb-storage/src/session.rs(Lines 1388-1397) callsfinish_mandatorywithout asserting readiness itself; the assertion runs earlier inAcceptedTableDdlScope::mark_terminal_ready. Calling the check inside the publication keeps the two steps inseparable.♻️ Proposed change
pub(crate) fn publish_mandatory_terminal(&self) { - self.inner.lock().state = SessionOperationState::Terminal; + let mut inner = self.inner.lock(); + assert!( + inner.state == SessionOperationState::Mandatory(None) + && inner.trx_id.is_none() + && inner.trx_inner.is_none(), + "mandatory terminal publication requires empty accepted authority: key={}, state={}", + self.key, + inner.state.label() + ); + inner.state = SessionOperationState::Terminal; }🤖 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 1836 - 1840, Update publish_mandatory_terminal to invoke assert_mandatory_finish_ready immediately before setting the state to Terminal, keeping validation and publication inseparable while preserving the existing terminal-state assignment.
1769-1783: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the unreachable arm explicit instead of returning
None.Lines 1766-1768 already take the transaction core and clear
trx_idbefore this match.completion_ownedis true only forMandatory(Some(Completing)),Voluntary(Some(Completing)), orCompleting, so_ => Noneis unreachable. If a future state variant reaches it, the entry loses its payload while the caller observes "no completion" and never finalizes the operation. Panic with the observed state instead.♻️ Proposed change
- _ => None, + state => panic!( + "owned transaction completion requires a completing state: key={}, state={}", + self.key, + state.label() + ),🤖 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 1769 - 1783, Update the match in the transaction completion logic around completion_owned to replace the unreachable _ => None arm with an explicit panic that includes the observed state, ensuring unexpected variants cannot silently report no completion after trx_id is cleared.doradb-storage/src/session.rs (1)
1962-1984: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a debug assertion that
entryis the active slot entry.
accept_mandatoryandfinish_mandatorytake the lifecycle lock but do not check thatentryis pointer-identical to the entry stored inSessionOperationSlot::Active. The doc comment states this invariant. Adebug_assert!makes the invariant executable and protects future callers.🛡️ Proposed debug assertion
fn accept_mandatory(&self, entry: &Arc<SessionOperationEntry>) { let lifecycle = self.lifecycle.lock(); + debug_assert!( + matches!(&lifecycle.slot, SessionOperationSlot::Active(active) if Arc::ptr_eq(active, entry)), + "mandatory acceptance requires the exact active slot entry: key={}", + entry.key() + ); entry.accept_mandatory();🤖 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/session.rs` around lines 1962 - 1984, Add debug-only pointer-identity assertions in accept_mandatory and finish_mandatory while holding the lifecycle lock, verifying that the supplied entry matches the entry stored in the active SessionOperationSlot state. Keep the existing ownership, publication, and notification behavior unchanged.doradb-storage/src/trx/stmt.rs (1)
892-909: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the two authority checks into one branch.
The method tests
self.prepared_catalog_writetwice, once at Line 892 and once at Line 905, separated only by validation. A single branch that acquires both locks, or asserts once, states the policy in one place.catalog_delete_primary_key_mvcc_innerat Lines 946-963 repeats the same shape.♻️ Proposed change
- if let Some(authority) = self.prepared_catalog_write { - authority.assert_table_write(table_id); - } else { - self.acquire_table_write_metadata_lock(table_id) - .await - .attach_with(|| format!("operation={OPERATION}, table_id={table_id}"))?; - } if !self.disable_dml_validation { DmlValidator::new(table.metadata()) .validate_full_row(&cols) .change_context(OperationError::InvalidDmlInput) .attach_with(|| format!("operation={OPERATION}, table_id={table_id}"))?; } - if self.prepared_catalog_write.is_none() { + if let Some(authority) = self.prepared_catalog_write { + authority.assert_table_write(table_id); + } else { + self.acquire_table_write_metadata_lock(table_id) + .await + .attach_with(|| format!("operation={OPERATION}, table_id={table_id}"))?; self.acquire_table_write_data_lock(table_id) .await .attach_with(|| format!("operation={OPERATION}, table_id={table_id}"))?; }Note: this moves validation before lock acquisition. If the existing order (metadata lock, then validation, then data lock) is deliberate, keep the order and store the authority in a local instead.
🤖 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 892 - 909, Collapse the duplicate self.prepared_catalog_write checks in the current method into one branch while preserving the existing metadata-lock, validation, and data-lock order; store the authority state in a local if needed rather than moving validation. Apply the same consolidation to catalog_delete_primary_key_mvcc_inner, keeping prepared-authority assertion versus lock acquisition behavior unchanged.doradb-storage/src/catalog/table.rs (2)
4311-4311: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the expected lock counts from the write-target arrays.
The counts 9 and 12 are hardcoded. They equal
1 + 2 * create_table_catalog_write_targets().len()and2 + 2 * drop_table_catalog_write_targets().len(). If a catalog table is added to either array, the test fails with an opaque number instead of pointing at the cause.♻️ Proposed change
- assert_eq!(lock_entry_count(&engine, create_owner), 9); + let expected_create_locks = 1 + 2 * create_table_catalog_write_targets().len(); + assert_eq!(lock_entry_count(&engine, create_owner), expected_create_locks);Also applies to: 4372-4372
🤖 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/table.rs` at line 4311, Update the lock-count assertions in the affected table catalog test to derive expected values from create_table_catalog_write_targets().len() and drop_table_catalog_write_targets().len(), preserving the existing 1/2 base offsets and 2x multipliers instead of hardcoding 9 and 12.
37-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse named catalog table constants instead of raw slot indices.
The arrays encode the catalog write set as slot numbers. The relationship between each slot and the catalog table written by
execute_create_table_catalog_stagingandexecute_drop_table_catalog_cascadeis implicit. Named constants make the lock coverage auditable when the cascade changes.♻️ Example shape
-const DROP_TABLE_CATALOG_WRITE_TARGETS: [TableID; 5] = [ - catalog_table_id_from_slot(0), - catalog_table_id_from_slot(1), - catalog_table_id_from_slot(2), - catalog_table_id_from_slot(3), - catalog_table_id_from_slot(4), -]; +const DROP_TABLE_CATALOG_WRITE_TARGETS: [TableID; 5] = [ + TABLE_ID_TABLES, + TABLE_ID_COLUMNS, + TABLE_ID_INDEXES, + TABLE_ID_INDEX_COLUMNS, + TABLE_ID_TABLE_REPLAY_SILENT_WATERMARKS, +];🤖 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/table.rs` around lines 37 - 49, Replace the raw slot indices in CREATE_TABLE_CATALOG_WRITE_TARGETS and DROP_TABLE_CATALOG_WRITE_TARGETS with the named catalog table constants corresponding to the tables written by execute_create_table_catalog_staging and execute_drop_table_catalog_cascade. Preserve the existing target sets and ordering while making each lock target explicit and auditable.
🤖 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/catalog/table.rs`:
- Around line 1375-1388: Preserve the primary failure when cleanup also fails in
both accepted DDL state machines: in
doradb-storage/src/catalog/table.rs:1375-1388, capture the begin_private_trx
error’s debug string and attach it as source_error to the provisional-file
cleanup error; in doradb-storage/src/catalog/table.rs:1683-1694, capture the
start_drop_lifecycle error and attach it to the rollback_catalog_ddl error.
Follow the existing abort_before_catalog_commit pattern so the original
rejection reason remains available.
In `@doradb-storage/src/table/layout.rs`:
- Around line 295-310: In the test flow around create_table2_for_test and the
purge_event_rx loop, explicitly request a purge cycle by sending
Purge::FullObservation (or the appropriate existing purge-request variant)
before awaiting CycleCompleted. Keep the existing CommittedRecorded gating and
event handling unchanged.
In `@doradb-storage/src/trx/sys.rs`:
- Around line 1130-1142: Add the explicit-session-lock validation to both index
DDL flows, create_index_for_session and drop_index_for_session, immediately
around their begin_private_trx setup. Call
reject_table_ddl_explicit_session_lock with the target table_id and
operation_lock_owner(), preserving the existing transaction behavior while
rejecting explicitly locked tables.
---
Nitpick comments:
In `@doradb-storage/src/catalog/table.rs`:
- Line 4311: Update the lock-count assertions in the affected table catalog test
to derive expected values from create_table_catalog_write_targets().len() and
drop_table_catalog_write_targets().len(), preserving the existing 1/2 base
offsets and 2x multipliers instead of hardcoding 9 and 12.
- Around line 37-49: Replace the raw slot indices in
CREATE_TABLE_CATALOG_WRITE_TARGETS and DROP_TABLE_CATALOG_WRITE_TARGETS with the
named catalog table constants corresponding to the tables written by
execute_create_table_catalog_staging and execute_drop_table_catalog_cascade.
Preserve the existing target sets and ordering while making each lock target
explicit and auditable.
In `@doradb-storage/src/session.rs`:
- Around line 1962-1984: Add debug-only pointer-identity assertions in
accept_mandatory and finish_mandatory while holding the lifecycle lock,
verifying that the supplied entry matches the entry stored in the active
SessionOperationSlot state. Keep the existing ownership, publication, and
notification behavior unchanged.
In `@doradb-storage/src/trx/mod.rs`:
- Around line 1836-1840: Update publish_mandatory_terminal to invoke
assert_mandatory_finish_ready immediately before setting the state to Terminal,
keeping validation and publication inseparable while preserving the existing
terminal-state assignment.
- Around line 1769-1783: Update the match in the transaction completion logic
around completion_owned to replace the unreachable _ => None arm with an
explicit panic that includes the observed state, ensuring unexpected variants
cannot silently report no completion after trx_id is cleared.
In `@doradb-storage/src/trx/stmt.rs`:
- Around line 892-909: Collapse the duplicate self.prepared_catalog_write checks
in the current method into one branch while preserving the existing
metadata-lock, validation, and data-lock order; store the authority state in a
local if needed rather than moving validation. Apply the same consolidation to
catalog_delete_primary_key_mvcc_inner, keeping prepared-authority assertion
versus lock acquisition behavior unchanged.
🪄 Autofix (Beta)
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: 1e9cf194-dbf7-403f-acd2-83ae0d261475
⛔ Files ignored due to path filters (8)
docs/benchmark-tool.mdis excluded by none and included by nonedocs/engine-component-lifetime.mdis excluded by none and included by nonedocs/lock-system.mdis excluded by none and included by nonedocs/public-error-audit.csvis excluded by!**/*.csvand included by nonedocs/table-file.mdis excluded by none and included by nonedocs/tasks/000249-runtime-owned-table-ddl.mdis excluded by none and included by nonedocs/tasks/next-idis excluded by none and included by nonedocs/transaction-system.mdis excluded by none and included by none
📒 Files selected for processing (11)
doradb-storage/src/catalog/table.rsdoradb-storage/src/completion.rsdoradb-storage/src/engine.rsdoradb-storage/src/lock/mod.rsdoradb-storage/src/runtime/mandatory.rsdoradb-storage/src/session.rsdoradb-storage/src/table/layout.rsdoradb-storage/src/table/persistence.rsdoradb-storage/src/trx/mod.rsdoradb-storage/src/trx/stmt.rsdoradb-storage/src/trx/sys.rs
💤 Files with no reviewable changes (2)
- doradb-storage/src/completion.rs
- doradb-storage/src/lock/mod.rs
1 participant
Closes #924
Summary by CodeRabbit
New Features
Bug Fixes
Tests