Extended `balance_root` to handle root underflow case #621

krishvishal · 2025-01-05T14:37:55Z

Balance root now handles root underflow case:

Root becomes underflow when its empty. Its a special case.
We try and copy root's child into root.
If successful we delete the child and update pointers.
If unsuccessful because the child can't fit inside the root, we early return and handle the child overflow in balance_leaf

Did some code refactoring.

… trying to convert overflow pages to PageType variant I have changed places which use `page_type` function to use `maybe_page_type`

krishvishal · 2025-01-05T19:14:36Z

I'm not sure why the test is failing only on windows. I have fixed the same error on my ubuntu machine. This is the only check that's failing.

…to root

core/storage/btree.rs

pereman2

Why do we handle root underflow? Balance root tries to map 1:1 to balance_deeper implementation which is an overflow balancing of root. Underflow should be managed generically for all cases. Furthermore, can a root page underflow even? Isn't it possible to create a btree page that once you do first insertion it will see itself as underfull?

krishvishal · 2025-01-08T18:24:03Z

@pereman2 I've looked at both the references below and thought it is better to handle root underflow case in the balance_root itself.

I think the comment from [1] describes root underflow case.

    /* The root page of the b-tree now contains no cells. The only sibling
    ** page is the right-child of the parent. Copy the contents of the
    ** child page into the parent, decreasing the overall height of the
    ** b-tree structure by one. This is described as the "balance-shallower"
    ** sub-algorithm in some documentation.
    **
    ** If this is an auto-vacuum database, the call to copyNodeContent()
    ** sets all pointer-map entries corresponding to database image pages
    ** for which the pointer is stored within the content being copied.
    **
    ** It is critical that the child page be defragmented before being
    ** copied into the parent, because if the parent is page 1 then it will
    ** by smaller than the child due to the database header, and so all the
    ** free space needs to be up front.

[1] https://github.com/sqlite/sqlite/blob/master/src/btree.c#L8867-L8881
[2]https://www.sqlite.org/btreemodule.html#:~:text=4.2.5.2%20Balance%20Shallower

core/storage/btree.rs

pereman2 · 2025-01-08T20:11:33Z

core/storage/btree.rs

@@ -1285,8 +1400,7 @@ impl BTreeCursor {

        // setup overflow page
        let contents = page.get().contents.as_mut().unwrap();
-        let buf = contents.as_ptr();
-        buf.fill(0);
+        contents.write_u32(0, 0); // Initialize next overflow page pointer to 0


Why 0, can we use one of the constants? This 0 means PAGE_HEADER_OFFSET_PAGE_TYPE, are you sure this is what this 0 write means?

Yes, what I did is wrong. Will fix it.

@pereman2 can you please tell me what's happening here?: https://github.com/sqlite/sqlite/blob/c2e400af042bdd7d21e159a41fcf34c05398044c/src/btree.c#L7166-L7172

This is my understanding:

// Writes the page number of the new overflow page (pgnoOvfl) to pPrior. For the first overflow page, pPrior points to the end of the local cell content. put4byte(pPrior, pgnoOvfl); // ignore this releasePage(pToRelease); pToRelease = pOvfl; // Points pPrior to the start of the new overflow page's data area pPrior = pOvfl->aData; // Writes 0 at the start of the overflow page, which shows its the last overflow page put4byte(pPrior, 0); // Sets pPayload to point past the 4-byte pointer. This is where the actual data will be stored in the overflow page pPayload = &pOvfl->aData[4]; //Calculates remaining space for data in this overflow page. Usable space minus 4 bytes used for the next-page pointer spaceLeft = pBt->usableSize - 4;

Where are we doing put4byte(pPrior, 0); equivalent in limbo?

1. Added an unreachable!() to the if-else under/overflow checker. 2. Directly inserted to cells using `insert_into_cell()` instead of cloning vecs into an external vector 3. Propagated errors using `?` instead of `except` and handled the case where child page is not loaded/locked. 4. Restored `allocate_overflow_page()` function to previous state.

krishvishal · 2025-01-11T14:54:10Z

@pereman2 @jussisaurio I've added the following changes, can you please take a look?

Added an unreachable!() to the if-else under/overflow checker.
Directly inserted to cells using insert_into_cell() instead of cloning vecs into an external vector
Propagated errors using ? instead of except and handled the case where child page is not loaded/locked.
Restored allocate_overflow_page() function to previous state.

…h its overflow cells to dst 2. Added `zero_page` function which zeros's page properly. 3. Fixed the bug on windows which was due to bad offset handling and the test_sequential_overflow_page passes now. 4. Now `balance_root` mirror's closely what sqlite is doing.

krishvishal · 2025-01-14T20:34:56Z

I think this is ready for review.

Added copy_node_content function which copies a page's along with its overflow cells to dst
Added zero_page function which zeros's page properly.
Fixed the bug on windows which was due to bad offset handling and the test_sequential_overflow_page passes now on Windows.
Now balance_root mirror's closely what sqlite does.

I will add tests for root underflow case when delete is functional.

pereman2 · 2025-01-20T09:05:53Z

core/storage/sqlite3_ondisk.rs

+        match self.maybe_page_type() {
+            Some(page_type) => match page_type {
+                PageType::IndexInterior => Some(self.read_u32(8)),
+                PageType::TableInterior => Some(self.read_u32(8)),
+                PageType::IndexLeaf => None,
+                PageType::TableLeaf => None,
+            },
+            None => None, // Handle overflow pages by returning None
        }


Why do we want to return None? Wouldn't it be better to crash if we call any of these methods if we are on an overflow page?

Removing these changes. I have made them for compute_used_space which is wrong and not needed for root balancing.

pereman2 · 2025-01-20T09:15:49Z

core/storage/btree.rs

+    fn zero_page(&self, page: &PageRef, page_type: PageType, is_leaf: bool) {
+        let contents = page.get().contents.as_mut().unwrap();


What if page is page 1?

In my understanding, since write_u16 and other write functions add page header size offsets when writing data to a location we need not handle page1 specially. Is my understanding correct?

pereman2 · 2025-01-20T09:16:53Z

core/storage/btree.rs

+        let header_and_pointers_size =
+            src_contents.header_size() + src_contents.cell_pointer_array_size();
+        dst_buf[..header_and_pointers_size].copy_from_slice(&src_buf[..header_and_pointers_size]);
+
+        // Copy overflow cells
+        dst_contents.overflow_cells = src_contents.overflow_cells.clone();


does this handle page 1?

Here we would need to add 100 as offset if page_1, since we are directly copying. I will make the necessary edits.

pereman2 · 2025-01-20T10:41:53Z

core/storage/btree.rs

+        for overflow_cell in &page.overflow_cells {
+            total_used += overflow_cell.payload.len();
+        }


Do overflow cells count? I mean, no overflow cell is there inside the page. If there is an overflow cell, regardless of the used space we have to balance.

This calculation is not needed for now and the calculation is wrong. I'm using this function inside in is_underflow. I've simplified both is_underflow and is_overflow to handle only balance_root case for. I will extend them in the future.

I will remove this function. This can be resolved.

…contents. 2. Refactored `zero_page` to only zero offsets. And added documentation on where and how to use it. 3. Removed erroneous `compute_used_space` function and changed `is_underflow` `is_overflow` to currently only check inside balance_root. 4. Removed changes to `maybe_page_type` function inside sqlite3_ondisk.

krishvishal · 2025-01-21T19:39:11Z

core/storage/btree.rs

+    fn zero_page(&self, page: &PageRef) {
+        let contents = page.get().contents.as_mut().unwrap();
+
+        contents.write_u32(PAGE_HEADER_OFFSET_FIRST_FREEBLOCK, 0);


@pereman2 I've written write_u32 here instead of write_u16 because SQLite does memset to 4 bytes: https://github.com/sqlite/sqlite/blob/c2e400af042bdd7d21e159a41fcf34c05398044c/src/btree.c#L2277

Extended balance_root to handle root underflow case

a1bd833

Did some code refactoring.

krishvishal marked this pull request as ready for review January 5, 2025 14:38

penberg requested review from pereman2 and jussisaurio January 5, 2025 18:13

Fixed the error due occuring due to PageContent::page_type function…

8a54812

… trying to convert overflow pages to PageType variant I have changed places which use `page_type` function to use `maybe_page_type`

krishvishal force-pushed the balance-root branch from cd7c875 to 8a54812 Compare January 5, 2025 18:43

In root underflow case, child page is defragmented before pushing it …

0f682f1

…to root

jussisaurio reviewed Jan 8, 2025

View reviewed changes

core/storage/btree.rs Outdated Show resolved Hide resolved

jussisaurio reviewed Jan 8, 2025

View reviewed changes

core/storage/btree.rs Outdated Show resolved Hide resolved

pereman2 requested changes Jan 8, 2025

View reviewed changes

krishvishal added 2 commits January 11, 2025 20:12

cargo fmt

abed8c9

krishvishal requested review from jussisaurio and pereman2 January 16, 2025 12:14

pereman2 reviewed Jan 20, 2025

View reviewed changes

krishvishal commented Jan 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extended `balance_root` to handle root underflow case #621

Extended `balance_root` to handle root underflow case #621

krishvishal commented Jan 5, 2025

krishvishal commented Jan 5, 2025

pereman2 left a comment

krishvishal commented Jan 8, 2025

pereman2 Jan 8, 2025

krishvishal Jan 11, 2025

krishvishal Jan 11, 2025

krishvishal commented Jan 11, 2025

krishvishal commented Jan 14, 2025

pereman2 Jan 20, 2025

krishvishal Jan 21, 2025

pereman2 Jan 20, 2025

krishvishal Jan 21, 2025

pereman2 Jan 20, 2025

krishvishal Jan 21, 2025

pereman2 Jan 20, 2025

krishvishal Jan 21, 2025

krishvishal Jan 21, 2025

		fn zero_page(&self, page: &PageRef, page_type: PageType, is_leaf: bool) {
		let contents = page.get().contents.as_mut().unwrap();

Extended balance_root to handle root underflow case #621

Are you sure you want to change the base?

Extended balance_root to handle root underflow case #621

Conversation

krishvishal commented Jan 5, 2025

krishvishal commented Jan 5, 2025

pereman2 left a comment

Choose a reason for hiding this comment

krishvishal commented Jan 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krishvishal commented Jan 11, 2025

krishvishal commented Jan 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Extended `balance_root` to handle root underflow case #621

Extended `balance_root` to handle root underflow case #621