perf: Storage and Executor optimizations for SQLite-parity#20
Merged
Conversation
Introduced fetch_page_by_id and unpin_page_by_id to bypass string-based file name lookups, significantly reducing latch contention on the hot path.
Cached file_id in HeapTable to avoid repeated hashes. Refactored HeapTable::Iterator to hold the current page pinned during sequential scans, reducing BPM pin/unpin overhead.
Added batch_insert_mode to QueryExecutor to skip locking and undo-log overhead during high-performance data loading. Exposed build_plan to allow cursor-like iteration in performance benchmarks.
Updated benchmarks to use batch mode and cursor iteration for cloudSQL, ensuring a fair apples-to-apples comparison with SQLite. Updated documentation with latest performance metrics showing ~60x faster inserts.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (3)
📝 WalkthroughWalkthroughIntroduces file-ID-based buffer-pool APIs, pins pages during heap-table iteration, adds a batch-insert mode to QueryExecutor, exposes plan-building publicly, and refactors benchmarks/docs to exercise and report these optimizations (plan reuse with arena resets, SQLite transaction wrapping). Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Executor as QueryExecutor
participant Plan as ExecutionPlan
participant Heap as HeapTable
participant BP as BufferPoolManager
participant Arena as Arena
Note over Client,Executor: Scan with plan reuse + pinned pages
Client->>Executor: request SELECT
Executor->>Plan: build_plan(SelectStatement)
Executor->>Arena: set_memory_resource(&arena)
loop per benchmark iteration
Executor->>Plan: root->init()
Executor->>Plan: root->open()
loop per tuple
Plan->>Heap: Iterator::next()
Heap->>BP: fetch_page_by_id(file_id, filename, page_id)
BP-->>Heap: Page*
Heap-->>Plan: Tuple
Plan->>Executor: yield tuple
end
Executor->>Plan: root->close()
Executor->>Arena: arena.reset()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/executor/query_executor.cpp (1)
214-220:⚠️ Potential issue | 🔴 CriticalDon't skip INSERT undo logging in batch mode.
This new guard breaks rollback semantics.
TransactionManager::undo_transaction()only removes inserted rows fromtxn->get_undo_logs()(src/transaction/transaction_manager.cpp:160-210), so aROLLBACK/abort after any successful insert in batch mode will leave those tuples committed in place.Proposed fix
- if (txn != nullptr && !batch_insert_mode_) { - txn->add_undo_log(transaction::UndoLog::Type::INSERT, prepared.table_meta->name, - tid); - if (!lock_manager_.acquire_exclusive(txn, tid)) { - throw std::runtime_error("Failed to acquire exclusive lock"); - } - } + if (txn != nullptr) { + txn->add_undo_log(transaction::UndoLog::Type::INSERT, prepared.table_meta->name, + tid); + if (!batch_insert_mode_ && !lock_manager_.acquire_exclusive(txn, tid)) { + throw std::runtime_error("Failed to acquire exclusive lock"); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/executor/query_executor.cpp` around lines 214 - 220, The current guard skips calling txn->add_undo_log(...) when batch_insert_mode_ is true, which breaks rollback because TransactionManager::undo_transaction() relies on INSERT undo logs to remove inserted tuples; restore undo logging for INSERTs even in batch_insert_mode_ by always invoking txn->add_undo_log(transaction::UndoLog::Type::INSERT, prepared.table_meta->name, tid) (but you can keep skipping exclusive locking via lock_manager_.acquire_exclusive when batch_insert_mode_ is true), ensuring undo entries are recorded so TransactionManager::undo_transaction() can remove rows on abort.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmarks/sqlite_comparison_bench.cpp`:
- Around line 177-191: The reusable scan benchmark assumes ParseSQL returns a
parser::SelectStatement and ctx.executor->build_plan(...) and
root->init()/root->open() succeed; add checks after ParseSQL to ensure the
returned pointer can be cast to parser::SelectStatement before using it, verify
that ctx.executor->build_plan(...) returns a non-null root, and check the
boolean results of root->init() and root->open() before entering the loop; if
any of these fail, call state.SkipWithError with a descriptive message and avoid
dereferencing root or running the benchmark loop.
In `@docs/performance/SQLITE_COMPARISON.md`:
- Around line 16-19: The analysis in Section 4 of SQLITE_COMPARISON.md is
inconsistent with the updated benchmark table (Point Inserts: cloudSQL Post-Opt
6.69M vs SQLite 114.1k; Sequential Scan: SQLite 20.6M vs cloudSQL Post-Opt 5.1M)
— remove or correct the outdated claims about a 7.1x insert gap, a 6.5x scan
gap, and per-row INSERT reparsing; update the narrative, conclusions, and any
comparison sentences to reflect the new numeric ratios (e.g., CloudSQL now
showing a +58x advantage on Point Inserts and SQLite ~4.0x advantage on Scans),
and ensure any explanations reference the new benchmark harness and numbers for
"Point Inserts", "Sequential Scan", and "Final Status".
In `@src/storage/buffer_pool_manager.cpp`:
- Around line 56-59: get_file_id() was changed to lock latch_ internally which
causes self-deadlock when callers like flush_page() and delete_page() already
hold latch_; revert the internal lock by removing the scoped_lock from
get_file_id (and the analogous wrappers at the other occurrences) so that
get_file_id simply calls get_file_id_internal(file_name) without taking latch_;
ensure callers that need thread-safety take the latch_ before calling
get_file_id, and keep get_file_id_internal as the single internal implementation
that assumes the caller holds latch_.
In `@src/storage/heap_table.cpp`:
- Around line 82-96: The move-assignment in HeapTable::Iterator::operator=
incorrectly transfers current_page_/current_page_num_ across different HeapTable
instances because table_ is a reference and cannot be reassigned; fix by
refusing or handling moves between iterators of different tables: in operator=
check that &other.table_ == &table_ and if not, unpin other.current_page_ (using
table_.bpm_.unpin_page_by_id with other.file_id_/other.current_page_num_) or
better, assert/return without transferring state to prevent cross-table
ownership transfer; ensure you update/clear current_page_/current_page_num_ and
other.current_page_ consistently and that destructor and next_meta() will unpin
against the correct file_id_.
---
Outside diff comments:
In `@src/executor/query_executor.cpp`:
- Around line 214-220: The current guard skips calling txn->add_undo_log(...)
when batch_insert_mode_ is true, which breaks rollback because
TransactionManager::undo_transaction() relies on INSERT undo logs to remove
inserted tuples; restore undo logging for INSERTs even in batch_insert_mode_ by
always invoking txn->add_undo_log(transaction::UndoLog::Type::INSERT,
prepared.table_meta->name, tid) (but you can keep skipping exclusive locking via
lock_manager_.acquire_exclusive when batch_insert_mode_ is true), ensuring undo
entries are recorded so TransactionManager::undo_transaction() can remove rows
on abort.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30c00336-f6d5-4c69-9371-2ea450277c1f
📒 Files selected for processing (9)
.gitignorebenchmarks/sqlite_comparison_bench.cppdocs/performance/SQLITE_COMPARISON.mdinclude/executor/query_executor.hppinclude/storage/buffer_pool_manager.hppinclude/storage/heap_table.hppsrc/executor/query_executor.cppsrc/storage/buffer_pool_manager.cppsrc/storage/heap_table.cpp
- Fix deadlock in BufferPoolManager by using internal lookups directly. - Safe heap_table cross-iterator destruction. - Restore undo_log in query executor. - Add safety checks to execution benchmarks. - Align benchmark documentation.
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
1 participant
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
This PR implements several core optimizations to close the performance gap between cloudSQL and SQLite, resulting in ~60x faster writes and 1.7x faster scans.
Key Changes
Performance Results (Local M3 Pro)
Summary by CodeRabbit
New Features
Performance Improvements
Documentation
Chores