feat(storage): support catalog transaction and add global transaction structure #133
Conversation
…/txnManager trait
There was a problem hiding this comment.
Pull request overview
This PR introduces catalog transaction support and a unified global transaction structure that coordinates updates across both catalog and graph storage layers. The implementation follows a two-phase commit protocol where catalog changes are validated first, graph changes are committed, and then catalog changes are published using the same commit timestamp.
Key changes include:
- Introduction of
Transactionstruct that coordinates catalog and graph sub-transactions with shared timestamps and isolation levels - New versioned catalog implementation with MVCC support for schema operations
- Refactoring of timestamp and isolation level types from
minigu-transactiontominigu-commonfor shared access - Removal of trait-based abstractions (
Transaction,GraphTxnManager) in favor of concrete types
Reviewed changes
Copilot reviewed 42 out of 43 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| minigu/transaction/src/transaction.rs | Implements global Transaction with two-phase commit coordination between catalog and graph |
| minigu/transaction/src/manager.rs | Provides TransactionManager for creating unified transactions |
| minigu/catalog/src/txn/* | New catalog transaction infrastructure with versioned maps and MVCC |
| minigu/catalog/src/memory/* | Updates catalog implementations to support transactional operations |
| minigu/common/src/transaction.rs | Moves IsolationLevel enum to common for shared access |
| minigu/common/src/timestamp.rs | Moves timestamp types and error handling to common |
| minigu/storage/src/tp/transaction.rs | Removes Transaction trait, adds GraphTxnView for polymorphic access |
| minigu/storage/src/tp/memory_graph.rs | Updates graph operations to accept GraphTxnView instead of concrete types |
| minigu/context/src/graph.rs | Integrates TransactionManager into GraphContainer for unified transaction creation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| label_set: LabelSet, | ||
| edge_type: Arc<MemoryEdgeTypeCatalog>, | ||
| ) -> bool { | ||
| match self.edge_type_map.entry(label_set) { | ||
| Entry::Occupied(_) => false, | ||
| Entry::Vacant(e) => { | ||
| e.insert(edge_type); | ||
| true | ||
| let txn = match txn_manager().begin_transaction(IsolationLevel::Serializable) { | ||
| Ok(txn) => txn, |
There was a problem hiding this comment.
I realized that all of our metadata operations are wrapped inside an anonymous transaction. In that case, if a user starts a transaction block in the CLI, how should this part of the transaction be handled? Will metadata transactional operations conflict with data transactional operations?
minigu/catalog/src/provider.rs
Outdated
| ) -> CatalogResult<Option<DirectoryOrSchema>> { | ||
| let _ = txn; | ||
| self.get_child(name) | ||
| } |
There was a problem hiding this comment.
So, this get_child_txn will ignore child's txn?
There was a problem hiding this comment.
Why do we need to explicitly provide functions with the txn suffix?
| ) -> Result<Arc<CatalogTxn>, CatalogTxnError> { | ||
| let txn_id = if let Some(id) = txn_id { | ||
| global_transaction_id_generator().update_if_greater(id)?; | ||
| id | ||
| } else { | ||
| global_transaction_id_generator().next()? | ||
| }; |
There was a problem hiding this comment.
why should call global_transaction_id_generator().update_if_greater(id)?; here?
There was a problem hiding this comment.
When starting a transaction with a specific ID (e.g., during WAL replay), we must advance the global generator. This ensures that subsequent calls to next() (for new transactions) will allocate strictly higher IDs, preventing ID collisions.
| pub fn begin_transaction( | ||
| &self, | ||
| isolation_level: IsolationLevel, | ||
| ) -> Result<Transaction, TxnError> { | ||
| self.txn_mgr.begin_transaction(isolation_level) | ||
| } | ||
|
|
There was a problem hiding this comment.
I was wondering: many metadata operations use a pattern where a transaction is automatically started and then an attempt is made to commit it. In that case, what is the purpose of maintaining a separate transaction related to data inside graphContainer?
There was a problem hiding this comment.
Refer to the reply in the first comment.
| let txn = txn_manager() | ||
| .begin_transaction(IsolationLevel::Serializable) | ||
| .map_err(|e| CatalogError::External(Box::new(e)))?; | ||
| let res = self.get_child_txn(name, txn.as_ref()); | ||
| txn.abort().ok(); |
There was a problem hiding this comment.
use snapshot will be better？
|
LGTM |
2 participants
feat(storage): support catalog transaction and add global transaction structure
Type
feat: (new feature)fix: (bug fix)docs: (doc update)refactor: (refactor code)test: (test code)chore: (other updates)Scope
query: (query engine)parser: (frontend parser)planner: (frontend planner)optimizer: (query optimizer)executor: (execution engine)op: (operators)storage: (storage engine)mvcc: (multi version concurrency control)schema: (graph model and topology)tool: (tools)cli: (cli)sdk: (sdk)none: (N/A)Description
Issue: #
Checklist
masterbranch.