fix: oom bug caused by removing temporary data page number restriction

[thweetkomputer]

Here are some reminders before you submit the pull request

• Add tests for the change
• Document changes
• Reference the link of issue using fixes eloqdb/eloqstore#issue_id
• Reference the link of RFC if exists
• Pass ctest --test-dir build/tests/

Summary by CodeRabbit

• Documentation

  • Clarified buffer pool description terminology from "shared page buffer" to "cached index pages per shard."

• Refactor

  • Removed page count limits; page pool now grows dynamically without maximum constraints.
  • Simplified redistribute operations with streamlined return values.
  • Optimized index page allocation with lazy initialization.
  • Removed early validation checks in I/O operations for cleaner error handling.
  • Updated test buffer pool configurations.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Refactors page allocation and error handling across storage and tasks: removes Page's bool conversion, drops PagesPool capacity tracking and changes Extend to void, and simplifies BatchWriteTask::Redistribute to return a Page. Adjusts related allocation/read paths and tests to match the new semantics.

Changes

Cohort / File(s)	Summary
Options & Header docs include/kv_options.h	Comment updated: buffer_pool_size described as "cached index pages per shard" (doc-only change).
Page & Pool API include/storage/page.h, include/storage/mem_index_page.h	Removed Page::operator bool(); MemIndexPage ctor now explicit MemIndexPage(bool alloc = true).
Batch write API include/tasks/batch_write_task.h	BatchWriteTask::Redistribute signature changed from std::pair<Page, KvError> to Page.
PagesPool implementation src/storage/page.cpp	Removed max_pages_/total_pages_, Extend(size_t) now returns void; pool grows lazily without enforced cap; related assertions updated.
Data / Index page logic src/storage/data_page.cpp, src/storage/index_page_manager.cpp	DataPage::IsEmpty() now checks page_.Ptr() == nullptr; constructor and allocation paths simplified; index page reservation removed; IsFull() now computes size dynamically vs. options.
Async I/O & read paths src/async_io_manager.cpp	Removed early null/Allocate checks during init and read; read flows proceed to underlying ops rather than pre-checking Page truthiness.
Batch write logic src/tasks/batch_write_task.cpp	Redistribute now returns Page; callers updated to accept Page and removed many explicit null/OutOfMem guard branches and pair unpacking.
Tests tests/batch_write.cpp, tests/concurrency.cpp	Test buffer_pool_size values reduced/tightened; one test removed explicit buffer_pool_size setting; test data entries adjusted for new constraints.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• Revert "Introduce arena for mapping and add YieldToNextRound() to make WorkOneRound() faster" #221 — Touches batching/redistribution and overflow control-flow; overlaps with Redistribute and overflow handling edits.
• fix: lazily extent page pool to speed up startup and change index_buffer_pool_size to buffer_pool_size to better restrict memory usage #270 — Modifies the same page/index buffering and batch-write APIs (Page bool operator, PagesPool::Extend, Redistribute, buffer sizing).
• Fix TruncateDataPage to handle empty pages and tail maintenance #212 — Changes BatchWriteTask::TruncateDataPage and relates to tail/overflow handling edits.

Suggested reviewers

• eatbreads

Poem

🐇 I nudged the pages, made the pool less tight,
I swapped out pairs for single pages in flight,
No boolean tricks, just pointers in view,
Simpler hops forward — a carrot for you! 🥕

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description only contains an unchecked template checklist with no actual implementation details, objectives, or rationale provided.	Fill in the description with details about the OOM bug being fixed, explain the root cause, describe the solution approach, and reference relevant issues using 'fixes eloqdb/eloqstore#issue_id'.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main fix: addressing an out-of-memory bug by removing temporary data page number restrictions.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix_oom_zc

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/storage/index_page_manager.cpp (1)

91-96: Verify the memory sizing calculation aligns with actual page memory usage.

The IsFull() check multiplies index_pages_.size() by data_page_size, but this accounts only for the page buffers allocated from the PagePool. Each MemIndexPage object has significant per-object overhead—the struct itself contains pointers (next_, prev_, tbl_ident_), IDs, reference counts, and a WaitingZone member—which adds 80+ bytes per page. Additionally, the std::vector<std::unique_ptr<MemIndexPage>> container has its own overhead.

This underestimation means actual memory pressure can exceed the calculated threshold, potentially exhausting the buffer pool before IsFull() returns true.

🤖 Fix all issues with AI agents

In @src/tasks/batch_write_task.cpp:
- Around line 1100-1106: The allocation of Page new_page(true) can return a null
pointer and Ptr() is used unguarded in the memcpy; add an immediate null check
after creating new_page (check new_page.Ptr() or new_page.IsValid()) and handle
the failure path (e.g., log/error/throw or return an appropriate sentinel Page)
before calling std::memcpy; update the code around the Page allocation in this
function so the corner-case branch that copies cur_page.data() only runs when
new_page.Ptr() is non-null, otherwise perform the chosen failure handling.
- Line 1143: FinishDataPage() uses the Page returned by Redistribute() without
validating allocation, causing a null dereference in production; modify
FinishDataPage() to check the Page returned by Redistribute() (inspect
page.Ptr() or page.is_valid()) before constructing DataPageIter or calling
iter.HasNext(), and if allocation failed propagate an error/return a failure (or
nullptr/new_page error path) rather than proceeding; reference Redistribute(),
Page (from PagePool()->Allocate()), FinishDataPage(), DataPageIter, page.Ptr(),
and iter.HasNext() when adding the early null-check and error propagation.

🧹 Nitpick comments (1)

src/storage/page.cpp (1)

110-120: Consider making the extension batch size configurable.

The hardcoded 1024 pages (4MB at default page size) is a reasonable default, but could benefit from being a configurable option in KvOptions to allow tuning for different workload profiles.

💡 Optional: Make extension size configurable

 char *PagesPool::Allocate()
 {
     if (free_head_ == nullptr)
     {
-        Extend(1024);  // Extend the pool with 1024 pages if free list is empty.
+        // Extend the pool if free list is empty.
+        Extend(options_->page_pool_extend_size);
         assert(free_head_ != nullptr);
     }

Add to KvOptions:

uint32_t page_pool_extend_size = 1024;

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f74df59 and a9fdc71.

📒 Files selected for processing (10)

• include/kv_options.h
• include/storage/page.h
• include/tasks/batch_write_task.h
• src/async_io_manager.cpp
• src/storage/data_page.cpp
• src/storage/index_page_manager.cpp
• src/storage/page.cpp
• src/tasks/batch_write_task.cpp
• tests/batch_write.cpp
• tests/concurrency.cpp

💤 Files with no reviewable changes (2)

• tests/concurrency.cpp
• src/async_io_manager.cpp

🧰 Additional context used

🧬 Code graph analysis (5)

include/storage/page.h (1)

src/storage/page.cpp (2)

• Extend (95-108)
• Extend (95-95)

include/tasks/batch_write_task.h (1)

src/tasks/batch_write_task.cpp (4)

• Redistribute (1075-1144)
• Redistribute (1075-1076)
• Redistribute (1146-1221)
• Redistribute (1146-1148)

src/storage/index_page_manager.cpp (1)

src/tasks/batch_write_task.cpp (2)

• new_page (1067-1067)
• new_page (1699-1699)

src/storage/data_page.cpp (1)

include/storage/data_page.h (1)

• page_ (204-205)

src/tasks/batch_write_task.cpp (4)

include/storage/page.h (1)

• Page (30-49)

src/storage/page.cpp (4)

• Page (33-43)
• Page (45-48)
• Page (50-53)
• Page (66-69)

src/tasks/write_task.cpp (8)

• WritePage (59-64)
• WritePage (59-59)
• WritePage (66-71)
• WritePage (66-66)
• WritePage (73-80)
• WritePage (73-73)
• WritePage (82-124)
• WritePage (82-82)

src/storage/data_page.cpp (3)

• OverflowPage (518-522)
• OverflowPage (524-554)
• OverflowPage (556-559)

🪛 Cppcheck (2.19.0)

src/storage/page.cpp

[error] 102-102: Mismatching allocation and deallocation

(mismatchAllocDealloc)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: test

🔇 Additional comments (17)

include/kv_options.h (1)

42-45: LGTM!

The documentation update accurately reflects the semantic shift from a fixed-capacity buffer pool to a cache-oriented model for index pages.

include/storage/page.h (1)

62-62: LGTM!

The signature change from bool to void aligns with the shift to assertion-based validation and unbounded pool growth. Callers no longer need to handle allocation failures explicitly since the pool now grows dynamically.

include/tasks/batch_write_task.h (1)

52-60: LGTM!

The simplified return type from std::pair<Page, KvError> to Page is consistent with the broader refactoring pattern shifting from explicit error returns to assertion-based validation. The documentation accurately describes the new behavior.

src/storage/page.cpp (3)

100-102: Static analysis false positive: aligned_alloc with std::free is valid.

The cppcheck warning about mismatching allocation/deallocation is incorrect. Per C11/C++17 standards, memory allocated via aligned_alloc can be freed with free(). The UPtr correctly uses &std::free as the deleter.

---

85-93: LGTM!

The constructor initialization is clean with a sensible initial allocation strategy. Starting with buf_ring_size pages (or 1 minimum) provides a reasonable baseline for io_uring operations while enabling lazy growth.

---

95-108: Verify abort-on-OOM is acceptable behavior.

The assertion at line 101 (assert(ptr)) will abort the process if aligned_alloc fails. This is a deliberate shift from potentially returning an error. Ensure this fail-fast behavior is acceptable for all deployment scenarios.

src/storage/data_page.cpp (2)

39-42: LGTM!

The explicit page_.Ptr() == nullptr check is clearer than relying on an implicit bool conversion and aligns with the removal of operator bool() from the Page class.

---

524-554: LGTM!

The constructor correctly relies on the assertion-based validation in PagesPool::Allocate. If allocation fails, the program aborts before reaching this code, so page_.Ptr() is guaranteed valid when used.

src/storage/index_page_manager.cpp (1)

51-53: LGTM! Simplified allocation logic.

The change from explicit unique_ptr construction and push_back to emplace_back is cleaner and correctly assigns the newly created page to next_free.

tests/batch_write.cpp (4)

130-130: LGTM! Tightened buffer constraint for OOM test.

Setting buffer_pool_size = 4096 (equal to data_page_size) correctly constrains the buffer pool to a single MemIndexPage, which is necessary to trigger the OutOfMem condition being tested.

---

164-164: LGTM! Increased initial entries for abort test.

Using 4 entries with large values (3500 bytes each) ensures the first write establishes enough state to pin pages before the second write triggers the OOM condition.

---

190-191: LGTM! Reasonable buffer sizing for concurrent partition test.

The buffer pool of 4096 * 400 pages provides sufficient capacity to test 3000 partitions while still allowing some OOM conditions to occur under contention.

---

317-317: LGTM! Minimal buffer for stress test.

Setting buffer_pool_size = opts.data_page_size creates a single-page buffer that maximizes memory pressure, appropriately stressing the abort and cleanup paths.

src/tasks/batch_write_task.cpp (4)

1041-1042: LGTM! Simplified redistribution call.

The change from std::pair<Page, KvError> to direct Page return is cleaner. The Page class handles allocation internally, and failure can be detected by checking Page::Ptr() if needed.

---

1050-1050: LGTM! Direct page allocation.

Using Page(true) directly in SetPage is straightforward. Per the Page constructor in src/storage/page.cpp, passing true allocates from the page pool.

---

1371-1372: LGTM! Inlined OverflowPage construction.

Constructing OverflowPage directly in the WritePage call is cleaner than creating a temporary variable. The OverflowPage constructor (per src/storage/data_page.cpp lines 523-553) handles Page(true) allocation internally.

---

1396-1396: LGTM! Consistent with the pattern above.

Same inline construction pattern for OverflowPage without pointers.