Skip to content
This repository was archived by the owner on Oct 10, 2025. It is now read-only.

Merge free pages after rollback #5921

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Merge free pages after rollback #5921

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
95aac1c
Merge free pages after rollback
benjaminwinger Aug 21, 2025
46a036b
ci: auto code format
benjaminwinger Aug 22, 2025
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
2 changes: 2 additions & 0 deletions src/include/storage/free_space_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ class FreeSpaceManager {
// each checkpoint we remove any already-evicted pages.
void clearEvictedBufferManagerEntriesIfNeeded(BufferManager* bufferManager);

void mergeFreePages(FileHandle* fileHandle);

private:
PageRange splitPageRange(PageRange chunk, common::page_idx_t numRequiredPages);
void mergePageRanges(free_list_t newInitialEntries, FileHandle* fileHandle);
Expand Down
2 changes: 2 additions & 0 deletions src/include/storage/page_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ class PageManager : public PageAllocator {
void finalizeCheckpoint();
void rollbackCheckpoint() { freeSpaceManager->rollbackCheckpoint(); }

void mergeFreePages(FileHandle* fileHandle);

common::row_idx_t getNumFreeEntries() const { return freeSpaceManager->getNumEntries(); }
std::vector<PageRange> getFreeEntries(common::row_idx_t startOffset,
common::row_idx_t endOffset) const {
Expand Down
4 changes: 4 additions & 0 deletions src/storage/free_space_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,10 @@ void FreeSpaceManager::finalizeCheckpoint(FileHandle* fileHandle) {
uncheckpointedFreePageRanges.clear();
}

void FreeSpaceManager::mergeFreePages(FileHandle* fileHandle) {
mergePageRanges(std::move(uncheckpointedFreePageRanges), fileHandle);
Copy link
Contributor

@royi-luo royi-luo Aug 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Let's rename mergeFreePages to something like finalizeRollback (this in the page manager too)? I'd like the use case of this function to be very specific since in general we shouldn't we worrying about the structure of the FSM in other modules.

I'd also like to change this to only merge the existing page ranges. I don't want this function to truncate the data file (or modify uncheckpointedFreePageRanges, although in practice it should always be empty in this case) since I'd like to maintain the assumption that we don't touch the data file or anything persistent outside of checkpoint. Maybe you can just refactor mergePageRanges() into two functions and just call the one that only does the merging.

Copy link
Contributor Author

@benjaminwinger benjaminwinger Aug 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Let's rename mergeFreePages to something like finalizeRollback? I'd like the use case of this function to be very specific since in general we shouldn't we worrying about the structure of the FSM in other modules.

That sounds like a good idea to me

I don't want this function to truncate the data file ... since I'd like to maintain the assumption that we don't touch the data file or anything persistent outside of checkpoint.

What about for copies where we already touch the data file outside of checkpoints? E.g. in CopyNodeAfterPKErrorRollbackFlushedGroups the rollback occurs after we've written some of the data (even if none of the data is written successfully, we currently write to the node table before updating the index, where the primary key insertion fails in that test). I would think that particularly for duplicate PK errors (e.g. if you accidentally copy the same input twice) we wouldn't want to keep the newly added pages, since the user might not want to retry.
Though isn't this only being used when we've touched the data file? We usually delay allocating pages until we're ready to write them.

Copy link
Contributor

@royi-luo royi-luo Aug 22, 2025

Choose a reason for hiding this comment

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

I think it’s fine to leave the data as garbage, it should be either overwritten or truncated after the next checkpoint. Since we rollback local storage even if there is no COPY (e.g. after just issuing a rollback statement) I’d still like to keep those cases from touching disk if possible.

Copy link
Contributor

@royi-luo royi-luo Aug 22, 2025

Choose a reason for hiding this comment

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

maybe only calling finalizerollback if we detect changes in the fsm works (we already have the versioning in place). That might also be unnecessarily complicating things though, I’ll leave this decision to you.

}

void FreeSpaceManager::resetFreeLists() {
freeLists.clear();
numEntries = 0;
Expand Down
5 changes: 3 additions & 2 deletions src/storage/local_storage/local_storage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,9 @@ void LocalStorage::rollback() {
for (auto& optimisticAllocator : optimisticAllocators) {
optimisticAllocator->rollback();
}
auto* bufferManager = mm->getBufferManager();
PageManager::Get(clientContext)->clearEvictedBMEntriesIfNeeded(bufferManager);
auto& pageManager = *PageManager::Get(clientContext);
pageManager.mergeFreePages(pageManager.getDataFH());
pageManager.clearEvictedBMEntriesIfNeeded(mm->getBufferManager());
}

} // namespace storage
Expand Down
8 changes: 8 additions & 0 deletions src/storage/page_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,14 @@ void PageManager::freeImmediatelyRewritablePageRange(FileHandle* fileHandle, Pag
}
}

void PageManager::mergeFreePages(FileHandle* fileHandle) {
if constexpr (ENABLE_FSM) {
common::UniqLock lck{mtx};
freeSpaceManager->mergeFreePages(fileHandle);
++version;
}
}

void PageManager::serialize(common::Serializer& serializer) {
freeSpaceManager->serialize(serializer);
}
Expand Down