feat: add transaction support (BEGIN/COMMIT/ROLLBACK) with WAL recovery#137
Merged
aviralgarg05 merged 2 commits intoFeb 13, 2026
Merged
Conversation
github-actions
Bot
added
documentation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis pull request implements transaction support by adding BEGIN/COMMIT/ROLLBACK commands with write-ahead logging (WAL) for durability. Changes span the parser, planner, executor, storage engine, and CLI layers. Transaction state is tracked via WAL, with recovery logic to restore state after crashes. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Parser
participant Executor
participant WAL as WAL File
participant Storage as Storage Engine
Client->>Parser: BEGIN TRANSACTION
Parser->>Executor: ExecutionResult(BeginTransaction)
Executor->>Executor: Create TransactionState
Executor->>WAL: Store initial snapshot
Executor-->>Client: TransactionBegan { tx_id }
Client->>Parser: INSERT/UPDATE/DELETE
Parser->>Executor: Execute operation
Executor->>Storage: Perform write
Executor->>WAL: Record transaction write
Executor-->>Client: OperationResult { writes }
Client->>Parser: COMMIT
Parser->>Executor: ExecutionResult(CommitTransaction)
Executor->>WAL: Mark transaction as committed
Executor->>Storage: Flush all writes
Executor-->>Client: TransactionCommitted { tx_id, writes }
alt Crash Scenario
Executor->>Executor: Startup recovery
Executor->>WAL: Read uncommitted tx from WAL
Executor->>Storage: Restore from snapshot
Executor->>WAL: Clear WAL entry
end
Client->>Parser: ROLLBACK
Parser->>Executor: ExecutionResult(RollbackTransaction)
Executor->>Storage: Restore from snapshot
Executor->>WAL: Clear transaction entry
Executor-->>Client: TransactionRolledBack { tx_id }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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 |
Signed-off-by: unknown <010samarjeet@gamil.com>
samarjeet818
force-pushed
the
feat/add-explain-query-plan
branch
from
4934a75 to
c9861e9
Compare
github-actions
Bot
added
size/L
and removed
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nexum_core/src/executor/mod.rs (1)
85-91:⚠️ Potential issue | 🟠 MajorWrites within a transaction are immediately visible to reads — no isolation.
The linked issue
#22mentions "Provide isolation via MVCC or an initial simpler locking mechanism," but the current implementation applies writes directly to storage during a transaction. AnySELECT(even from a hypothetical concurrent session) will see uncommitted data. This is effectively a "dirty read" isolation level.Consider at minimum documenting this limitation, or buffering writes in the
TransactionStateand only applying them onCOMMIT.
🤖 Fix all issues with AI agents
In `@nexum_core/src/executor/mod.rs`:
- Around line 40-41: The tx_state field currently uses
RefCell<Option<TransactionState>> which makes Executor !Send and !Sync; replace
it with std::sync::Mutex<Option<TransactionState>> (keep tx_counter as
AtomicU64) so Executor becomes thread-compatible; update all uses of
tx_state.borrow()/borrow_mut() to tx_state.lock().unwrap() (or handle
PoisonError) and adjust any pattern matching or option manipulation accordingly
in the Executor implementation and methods that reference
tx_state/TransactionState.
- Around line 508-531: The BEGIN implementation in begin_transaction uses
storage.scan_all() to create a full in-memory and on-disk TxWalFile snapshot
(TxWalFile, write_transaction_wal, tx_state), which is O(n) and won't scale;
update the codebase by adding a clear TODO comment inside begin_transaction
explaining this known limitation, reference scan_all() and TxWalFile usage, and
add an entry to project documentation or CHANGELOG stating that the current WAL
is a full-snapshot MVP and must be replaced with an incremental undo/redo
mutation log; also create or link a tracking issue/placeholder task ID in the
comment pointing to a future refactor to per-mutation WAL so reviewers and
future contributors can find and prioritize the change.
- Around line 570-574: record_transaction_write currently no-ops outside a
transaction and increments tx_state.write_count by one per statement, but you
should change it to track actual row mutations: modify fn
record_transaction_write to accept a rows: usize parameter and add rows to
self.tx_state.borrow_mut().as_mut().write_count (preserving the no-op when
tx_state is None), then update all call sites that currently call
record_transaction_write() to pass the number of rows affected (e.g., where rows
are known in mutation handlers or execution paths), ensuring the symbol names
record_transaction_write, tx_state, and write_count are used so the compiler
guides you to adjust every usage.
- Around line 637-657: restore_snapshot currently deletes missing keys then
rewrites the entire snapshot (using snapshot.to_vec()), which is expensive and
clones the whole slice; change restore_snapshot to compute a diff: call
self.storage.scan_all() and build a HashMap from the incoming snapshot slice for
lookups, then build two small collections — keys_to_delete (keys present in
current but not in snapshot) and entries_to_set (only those (k,v) from snapshot
where either k is missing in current or value differs) — call
self.storage.delete_keys(&keys_to_delete)? and
self.storage.batch_set(entries_to_set)? (collect entries_to_set into a
Vec<(Vec<u8>,Vec<u8>)> by cloning only the changed/new entries, not the entire
snapshot), then self.storage.flush()?; update code around restore_snapshot,
scan_all, delete_keys, batch_set, and flush accordingly to avoid the
unconditional full rewrite and the snapshot.to_vec() clone.
- Around line 533-551: commit_transaction currently clones the entire
state.snapshot into TxWalFile just to mark the WAL entry committed and
immediately deletes it; remove that unnecessary clone by constructing the
committed TxWalFile without the snapshot (or with None) before calling
write_transaction_wal, so you still pass tx_id and committed: true but avoid
using state.snapshot.clone(); update references in commit_transaction to stop
reading state.snapshot.clone() and ensure write_transaction_wal and TxWalFile
can accept a missing/None snapshot for committed entries, then keep
clear_transaction_wal() and resetting tx_state as-is.
- Around line 611-623: The write_transaction_wal function currently uses
fs::write which doesn't fsync; change it to open the WAL path with File::create
(using the same path from self.storage.wal_path()), write the
serde_json::to_vec(wal) bytes via write_all, then call sync_all on the file to
flush to disk (and map any IO/serde errors to StorageError::WriteError as
before); keep the existing parent directory creation logic and error mapping,
but replace the single fs::write call with the create/write_all/sync_all
sequence to guarantee WAL durability for TxWalFile.
In `@nexum_core/src/sql/parser.rs`:
- Around line 217-234: The parser currently recognizes single-word "commit" and
"rollback" but not the two-token variants "commit transaction" and "rollback
transaction"; update the token checks in nexum_core::sql::parser (the matching
block that returns Statement::BeginTransaction / Statement::CommitTransaction /
Statement::RollbackTransaction) to mirror the existing two-token handling used
for "begin transaction" by adding conditions that check tokens.len() == 2 &&
tokens[0].eq_ignore_ascii_case("commit") &&
tokens[1].eq_ignore_ascii_case("transaction") to return
Statement::CommitTransaction and likewise tokens.len() == 2 &&
tokens[0].eq_ignore_ascii_case("rollback") &&
tokens[1].eq_ignore_ascii_case("transaction") to return
Statement::RollbackTransaction so the parser accepts both single- and two-token
forms.
In `@nexum_core/src/storage/engine.rs`:
- Around line 13-18: The WAL file is currently created inside sled's managed
directory (wal_path: Some(db_path.join("nexum_tx_wal.json"))) which can conflict
with sled; change the WAL path in the constructor to be a sibling in the parent
directory (derive parent = db_path.parent() and set wal_path to
parent.join("nexum_tx_wal.json"), handling the case where parent is None by
falling back to db_path or returning an error) so the file is outside sled's
directory; update the code that reads/writes wal_path accordingly (referencing
wal_path and the constructor that returns Self { db, wal_path }) and add unit
tests validating scan_all() and delete_keys() behavior to catch regressions.
Signed-off-by: unknown <010samarjeet@gamil.com>
github-actions
Bot
added
the
documentation
Contributor
Author
|
All checks are passing and there are no merge conflicts. |
aviralgarg05
added
OSCG26
medium
This was referenced Feb 15, 2026
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
2 participants
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
BEGIN,BEGIN TRANSACTION,COMMIT, andROLLBACKBEGINROLLBACKkey.as_slice()for sled compatibilityTesting
cargo fmt --all -- --checkcargo +stable-x86_64-pc-windows-gnu clippy --workspace --all-targets -- -D warningscargo +stable-x86_64-pc-windows-gnu doc --workspace --no-depsruff check nexum_aipython -m compileall -q nexum_aicd nexum_ai && pytest --cov=. --cov-report=xml --cov-report=term-missing(97 passed)Closes #22
Summary by CodeRabbit
New Features
Tests