Skip to content

fix(heap_table): TupleId sentinel bug and 28 unit tests #29

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
poyrazK merged 3 commits into from
Apr 13, 2026
Merged

fix(heap_table): TupleId sentinel bug and 28 unit tests #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f579051
fix(heap_table): TupleId sentinel bug and add 28 unit tests
poyrazK Apr 13, 2026
0f006ed
fix(heap_table): TupleId sentinel bug and add 28 unit tests
poyrazK Apr 13, 2026
615fa1c
style: automated clang-format fixes
poyrazK Apr 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ CMakeFiles/
cmake_install.cmake
Makefile
*.cmake
_deps/

# Compiled binaries
sqlEngine
Expand Down
1 change: 1 addition & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ if(BUILD_TESTS)
add_cloudsql_test(raft_group_tests tests/raft_group_tests.cpp)
add_cloudsql_test(raft_protocol_tests tests/raft_protocol_tests.cpp)
add_cloudsql_test(columnar_table_tests tests/columnar_table_tests.cpp)
add_cloudsql_test(heap_table_tests tests/heap_table_tests.cpp)
add_cloudsql_test(storage_manager_tests tests/storage_manager_tests.cpp)
add_cloudsql_test(rpc_server_tests tests/rpc_server_tests.cpp)
add_cloudsql_test(operator_tests tests/operator_tests.cpp)
Expand Down
4 changes: 2 additions & 2 deletions include/storage/heap_table.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,11 @@ class HeapTable {
uint32_t page_num; /**< Physical page index in the file */
uint16_t slot_num; /**< Logical slot index within the page */

TupleId() : page_num(0), slot_num(0) {}
TupleId() : page_num(UINT32_MAX), slot_num(0) {}
TupleId(uint32_t page, uint16_t slot) : page_num(page), slot_num(slot) {}

/** @return true if the ID represents a null/invalid record */
[[nodiscard]] bool is_null() const { return page_num == 0 && slot_num == 0; }
[[nodiscard]] bool is_null() const { return page_num == UINT32_MAX && slot_num == 0; }
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/** @return Human-readable string representation */
[[nodiscard]] std::string to_string() const {
Expand Down
6 changes: 6 additions & 0 deletions src/storage/heap_table.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,12 @@ bool HeapTable::get_meta(const TupleId& tuple_id, TupleMeta& out_meta) const {
std::memcpy(&out_meta.xmin, data + 2, 8);
std::memcpy(&out_meta.xmax, data + 10, 8);

/* Skip tuples that have been logically deleted */
if (out_meta.xmax != 0) {
bpm_.unpin_page_by_id(file_id_, tuple_id.page_num, false);
return false;
Comment on lines +546 to +549
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Do not unpin cached_page_ from this fast path.

On the cached-page branch, cached_page_ stays stored on the HeapTable after this return. Dropping the pin here without clearing that cache leaves a stale owned pointer behind, so later inserts or destruction can unpin/reuse a frame the table no longer owns.

Suggested fix 
         /* Skip tuples that have been logically deleted */
         if (out_meta.xmax != 0) {
-            bpm_.unpin_page_by_id(file_id_, tuple_id.page_num, false);
             return false;
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/* Skip tuples that have been logically deleted */
if (out_meta.xmax != 0) {
bpm_.unpin_page_by_id(file_id_, tuple_id.page_num, false);
return false;
/* Skip tuples that have been logically deleted */
if (out_meta.xmax != 0) {
return false;
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/storage/heap_table.cpp` around lines 546 - 549, The fast-path early
return unpins the page via bpm_.unpin_page_by_id(file_id_, tuple_id.page_num,
false) even when the page is stored in cached_page_, leaving cached_page_ as a
dangling/stale owned pointer; change the logic in the block that checks
out_meta.xmax != 0 to avoid unpinning a cached_page_ (either skip
bpm_.unpin_page_by_id when cached_page_ is set or first clear/reset cached_page_
before unpinning) so cached_page_ no longer holds a stale reference — update the
code around cached_page_, bpm_.unpin_page_by_id, file_id_, and tuple_id.page_num
accordingly.

}
Comment on lines +546 to +550
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Keep get_meta() as raw metadata access.

src/executor/operator.cpp Lines 200-228 still calls get_meta() and then applies snapshot visibility rules using meta.xmax. Returning false here makes deleted-but-still-visible versions disappear for older snapshots, and because the fallback path at Lines 598-677 does not do the same check, the result currently depends on whether the page happens to be cached. If plain reads should hide deleted tuples, move that policy into get() or a separate visibility-aware helper.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/storage/heap_table.cpp` around lines 546 - 550, The code in get_meta() is
making get_meta() hide logically-deleted tuples by checking out_meta.xmax and
returning false, which breaks callers that expect raw metadata (e.g.,
executor::operator uses get_meta() and applies snapshot rules). Revert this:
remove the deletion-check early return in the get_meta() path (and keep the page
unpin semantics correct) so get_meta() continues to return raw meta regardless
of xmax; then move the deleted-tuple filtering into the visibility-aware read
path (either heap_table::get() or a new helper like is_visible_under_snapshot())
and update callers (e.g., operator.cpp usage) to call that visibility helper or
let get() enforce plain-read hiding if desired.


size_t cursor = 18;
std::vector<common::Value> values;
values.reserve(schema_.column_count());
Expand Down
Loading