Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BTree delete implementation #661

Closed
wants to merge 13 commits into from

Conversation

krishvishal
Copy link
Contributor

@krishvishal krishvishal commented Jan 12, 2025

Get cell_idx from stack and drops the page

Currently the cell_idx we are getting is wrong (1 instead of 0 for a 1 page Btree). I suspect the problem is with seek which is advancing the stack's cell_indices[0] before delete is called. Check test/src/lib.rs::test_simple_delete() unit test.

  • If I move this self.stack.advance to after if found block then delete works and test passes.

@pereman2 need some guidance on this.


Core delete tasks:

  • Implement free page functionality
  • Clear overflow pages if any before deleting their cells using free page.
  • Implement delete for leaf page
  • Balance after delete properly

Auxiliary tasks to make delete work properly:

  • Implement block coalescing in free_cell_range to reduce fragmentation.
  • Track page fragmentation in free_cell_range.
  • Update page offsets in drop_cell and update cell pointer array after dropping a cell.

@krishvishal
Copy link
Contributor Author

Added PRAGMA freelist_count support.

Check the SQLite docs for more info: https://www.sqlite.org/pragma.html#pragma_freelist_count

COMPAT.md Outdated
@@ -45,6 +45,7 @@ This document describes the SQLite compatibility status of Limbo:
| ON CONFLICT clause | No | |
| PRAGMA | Partial | |
| PRAGMA cache_size | Yes | |
| PRAGMA freelist_count | Yes | |
Copy link
Contributor

@sonhmai sonhmai Jan 23, 2025

Choose a reason for hiding this comment

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

can we rebase with main and update the PRAGMA in another section?
in main we re-org the pragma statements in db3fa25

Copy link
Contributor

@sonhmai sonhmai left a comment

Choose a reason for hiding this comment

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

there are some changes in main that can cause conflict. can we rebase.

Comment on lines +508 to +513

#[test]
fn test_simple_delete() -> anyhow::Result<()> {
let _ = env_logger::try_init();
let tmp_db = TempDatabase::new("CREATE TABLE test (x INTEGER PRIMARY KEY);");
let conn = tmp_db.connect_limbo();
Copy link
Contributor

Choose a reason for hiding this comment

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

in main we also refactor this integration tests module a bit. can we rebase to avoid conflict here.

@krishvishal krishvishal changed the title BTree page delete implementation BTree delete implementation Jan 26, 2025
… found`. This removes the problem with wrong cell_indices

being fed to `delete()`.
…fter `if found`. This removes the problem with wrong cell_indices"

This reverts commit 6d077cc.
Now we can add dropped pages to free list, so pages can be reused in allocate_page.
… use `clear_overflow_cells` to remove any overflow pages it has and then add them to free list.
…ion is called on cell with no overflow pages.
Track page fragmentation in free_cell_range.

Update page offsets in drop_cell and update cell pointer array after dropping a cell.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants