From 67e260ff71fe3fabb15d58106a6fcc92ab4bbbaf Mon Sep 17 00:00:00 2001 From: Pere Diaz Bou Date: Mon, 19 May 2025 11:07:30 +0200 Subject: [PATCH] allow delete of dirty page in cacheflush Dirty pages can be deleted in `cacheflush`. Furthermore, there could be multiple live references in the stack of a cursor so let's allow them to exist while deleting. --- core/storage/btree.rs | 2 +- core/storage/page_cache.rs | 23 ++++----------- core/storage/pager.rs | 59 ++++++++++++++++++++++++++++---------- core/translate/pragma.rs | 4 ++- 4 files changed, 53 insertions(+), 35 deletions(-) diff --git a/core/storage/btree.rs b/core/storage/btree.rs index 6332fc06e..88897f4c2 100644 --- a/core/storage/btree.rs +++ b/core/storage/btree.rs @@ -2815,7 +2815,7 @@ impl BTreeCursor { if *new_id != page.get().id { page.get().id = *new_id; // FIXME: why would there be another page at this id/frame? - self.pager.put_loaded_page(*new_id, page.clone()); + self.pager.put_loaded_page(*new_id, page.clone())?; } } diff --git a/core/storage/page_cache.rs b/core/storage/page_cache.rs index 0d3047d9e..651637c70 100644 --- a/core/storage/page_cache.rs +++ b/core/storage/page_cache.rs @@ -26,12 +26,6 @@ struct PageCacheEntry { next: Option>, } -impl PageCacheEntry { - fn as_non_null(&mut self) -> NonNull { - NonNull::new(&mut *self).unwrap() - } -} - pub struct DumbLruPageCache { capacity: usize, map: RefCell>>, @@ -166,18 +160,9 @@ impl DumbLruPageCache { } if clean_page { - if let Some(page_mut) = Arc::get_mut(&mut entry_mut.page) { - page_mut.clear_loaded(); - debug!("cleaning up page {}", page_mut.get().id); - let _ = page_mut.get().contents.take(); - } else { - let page_id = unsafe { &entry.as_mut().page.get().id }; - debug!( - "detach page {}: can't clean, there are other references", - page_id - ); - return Err(CacheError::ActiveRefs); - } + entry_mut.page.clear_loaded(); + debug!("cleaning up page {}", entry_mut.page.get().id); + let _ = entry_mut.page.get().contents.take(); } self.unlink(entry); Ok(()) @@ -950,6 +935,7 @@ mod tests { } #[test] + #[should_panic(expected = "Attempted to insert different page with same key")] fn test_resize_larger() { let mut cache = DumbLruPageCache::new(2); let _ = insert_page(&mut cache, 1); @@ -965,6 +951,7 @@ mod tests { let _ = insert_page(&mut cache, i); } assert_eq!(cache.len(), 5); + // FIXME: For now this will assert because we cannot insert a page with same id but different contents of page. assert!(cache.insert(create_key(4), page_with_content(4)).is_err()); cache.verify_list_integrity(); } diff --git a/core/storage/pager.rs b/core/storage/pager.rs index 96e552b55..7d1acde7d 100644 --- a/core/storage/pager.rs +++ b/core/storage/pager.rs @@ -13,7 +13,7 @@ use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Arc; use tracing::trace; -use super::page_cache::{CacheError, DumbLruPageCache, PageCacheKey, CacheResizeResult}; +use super::page_cache::{CacheError, CacheResizeResult, DumbLruPageCache, PageCacheKey}; use super::wal::{CheckpointMode, CheckpointStatus}; pub struct PageInner { @@ -333,8 +333,15 @@ impl Pager { match page_cache.insert(page_key, page.clone()) { Ok(_) => {} Err(CacheError::Full) => return Err(LimboError::CacheFull), - Err(CacheError::KeyExists) => unreachable!("Page should not exist in cache after get() miss"), - Err(e) => return Err(LimboError::InternalError(format!("Failed to insert page into cache: {:?}", e))), + Err(CacheError::KeyExists) => { + unreachable!("Page should not exist in cache after get() miss") + } + Err(e) => { + return Err(LimboError::InternalError(format!( + "Failed to insert page into cache: {:?}", + e + ))) + } } return Ok(page); } @@ -348,8 +355,15 @@ impl Pager { match page_cache.insert(page_key, page.clone()) { Ok(_) => {} Err(CacheError::Full) => return Err(LimboError::CacheFull), - Err(CacheError::KeyExists) => unreachable!("Page should not exist in cache after get() miss"), - Err(e) => return Err(LimboError::InternalError(format!("Failed to insert page into cache: {:?}", e))), + Err(CacheError::KeyExists) => { + unreachable!("Page should not exist in cache after get() miss") + } + Err(e) => { + return Err(LimboError::InternalError(format!( + "Failed to insert page into cache: {:?}", + e + ))) + } } Ok(page) } @@ -373,9 +387,14 @@ impl Pager { page.set_uptodate(); } match page_cache.insert(page_key, page.clone()) { - Err(CacheError::KeyExists) => {}, // Exists but same page, not error + Err(CacheError::KeyExists) => {} // Exists but same page, not error Err(CacheError::Full) => return Err(LimboError::CacheFull), - Err(e) => return Err(LimboError::InternalError(format!("Failed to insert page into cache during load: {:?}", e))), + Err(e) => { + return Err(LimboError::InternalError(format!( + "Failed to insert page into cache during load: {:?}", + e + ))) + } Ok(_) => {} } return Ok(()); @@ -383,10 +402,15 @@ impl Pager { } match page_cache.insert(page_key, page.clone()) { - Err(CacheError::KeyExists) => {}, // Ensures same page - Err(CacheError::Full) => return Err(LimboError::CacheFull), - Err(e) => return Err(LimboError::InternalError(format!("Failed to insert page into cache during load: {:?}", e))), - Ok(_) => {} + Err(CacheError::KeyExists) => {} // Ensures same page + Err(CacheError::Full) => return Err(LimboError::CacheFull), + Err(e) => { + return Err(LimboError::InternalError(format!( + "Failed to insert page into cache during load: {:?}", + e + ))) + } + Ok(_) => {} }; sqlite3_ondisk::begin_read_page( @@ -440,8 +464,8 @@ impl Pager { for page_id in self.dirty_pages.borrow().iter() { let mut cache = self.page_cache.write(); let page_key = PageCacheKey::new(*page_id, Some(max_frame)); + let page = cache.get(&page_key).expect("we somehow added a page to dirty list but we didn't mark it as dirty, causing cache to drop it."); if let Some(wal) = &self.wal { - let page = cache.get(&page_key).expect("we somehow added a page to dirty list but we didn't mark it as dirty, causing cache to drop it."); let page_type = page.get().contents.as_ref().unwrap().maybe_page_type(); trace!("cacheflush(page={}, page_type={:?}", page_id, page_type); wal.borrow_mut().append_frame( @@ -450,6 +474,7 @@ impl Pager { self.flush_info.borrow().in_flight_writes.clone(), )?; } + page.clear_dirty(); // This page is no longer valid. // For example: // We took page with key (page_num, max_frame) -- this page is no longer valid for that max_frame @@ -579,7 +604,10 @@ impl Pager { } } // TODO: only clear cache of things that are really invalidated - self.page_cache.write().clear().expect("Failed to clear page cache"); + self.page_cache + .write() + .clear() + .expect("Failed to clear page cache"); checkpoint_result } @@ -731,7 +759,8 @@ impl Pager { cache.insert(page_key, page.clone()).map_err(|e| { LimboError::InternalError(format!( - "Failed to insert loaded page {} into cache: {:?}", id, e + "Failed to insert loaded page {} into cache: {:?}", + id, e )) })?; @@ -807,7 +836,7 @@ mod tests { std::thread::spawn(move || { let mut cache = cache.write(); let page_key = PageCacheKey::new(1, None); - cache.insert(page_key, Arc::new(Page::new(1))); + cache.insert(page_key, Arc::new(Page::new(1))).unwrap(); }) }; let _ = thread.join(); diff --git a/core/translate/pragma.rs b/core/translate/pragma.rs index 4e49a55d1..2de4ce996 100644 --- a/core/translate/pragma.rs +++ b/core/translate/pragma.rs @@ -305,5 +305,7 @@ fn update_cache_size(value: i64, header: Arc>, pager: R pager.write_database_header(&header_copy); // update cache size - pager.change_page_cache_size(cache_size); + pager + .change_page_cache_size(cache_size) + .expect("couldn't update page cache size"); }