storage/page_cache: Implement spilling to disk #3995
Conversation
jussisaurio
left a comment
jussisaurio
left a comment
There was a problem hiding this comment.
before I look at this more, submitting these comments since the use of return_if_io!() is problematic for async IO
core/storage/btree.rs
Outdated
| let past_rightmost_pointer = cell_count as i32 + 1; | ||
| self.stack.set_cell_index(past_rightmost_pointer); | ||
| let (page, c) = self.read_page(rightmost_pointer as i64)?; | ||
| let (page, c) = return_if_io!(self.read_page(rightmost_pointer as i64)); |
There was a problem hiding this comment.
This isn't re-entrant in async IO - i32::MAX is a sentinel value in backwards iteration to signify we have just started iterating this page backwards. If we:
self.stack.set_cell_index(past_rightmost_pointer);
// return IO here
let (page, c) = return_if_io!(self.read_page(rightmost_pointer as i64));
Then we never push the rightmost child page to the page stack and will have mutated stack.cell_index when we enter the function again.
We are already returning a completion here, is return_if_io! necessary?
There was a problem hiding this comment.
Ok I half-vibecoded, then fixed, making all of this reentrant... the fuzz tests that were failing previously are passing.
TBH I'm not sure exactly what the best ergonomics are for the cases where we are already returning a completion.. it doesn't fit exactly because now its an IOResult<(PageRef, Option<Completion>)>.
| } | ||
|
|
||
| let (mem_page, c) = self.read_page(left_child_page as i64)?; | ||
| let (mem_page, c) = return_if_io!(self.read_page(left_child_page as i64)); |
There was a problem hiding this comment.
same deal here - we might've just called self.stack.retreat() and then return on IO without pushing mem_page to the stack.
We are already returning a completion here, is return_if_io! necessary?
core/storage/btree.rs
Outdated
| loop { | ||
| if self.read_overflow_state.borrow().is_none() { | ||
| let (page, c) = self.read_page(start_next_page as i64)?; | ||
| let (page, c) = return_if_io!(self.read_page(start_next_page as i64)); |
There was a problem hiding this comment.
We are already returning a completion here, is return_if_io! necessary?
core/storage/btree.rs
Outdated
|
|
||
| if *remaining_to_read != 0 && next != 0 { | ||
| let (new_page, c) = self.pager.read_page(next as i64)?; | ||
| let (new_page, c) = return_if_io!(self.pager.read_page(next as i64)); |
There was a problem hiding this comment.
not re-entrant. We are already returning a completion here, is return_if_io! necessary?
| // Read page | ||
| let (page, c) = self.read_page(first_overflow_page.unwrap() as i64)?; | ||
|
|
||
| let (page, c) = return_if_io!(self.read_page(first_overflow_page.unwrap() as i64)); |
There was a problem hiding this comment.
We are already returning a completion here, is return_if_io! necessary?
|
|
||
| let (next_page, c) = self.read_page(next as i64)?; | ||
|
|
||
| let (next_page, c) = return_if_io!(self.read_page(next as i64)); |
There was a problem hiding this comment.
We are already returning a completion here, is return_if_io! necessary?
2 participants
This PR implements cache spilling so we can remove the ridiculous 100k page default cache size :)
Successfully tested on TPC-H queries with page_cache
size=100pagesMakes some attempt at preventing thrashing by tracking how many times a page is 'touched' in between 'clear_dirty' and counting that as 'dirty_generation', then intentionally spilling only the coldest pages first.
EDIT: Might need to go back through and self-review this tomorrow, as I haven't touched this in a couple weeks, I just fixed the last remaining bug and pushed.