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

Fix (btree) clear_overflow_page #971

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

krishvishal
Copy link
Contributor

This PR fixes bugs in clear_overflow_page implementation.

  • Added a correct way to get cell size information.
  • Added basic delete fuzzer.
  • Removed clones from different copy types and fixed some clippy warnings.

Copy link
Collaborator

@pereman2 pereman2 left a comment

Choose a reason for hiding this comment

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

left some minor comments

Comment on lines 2751 to 2773
log::info!(
"After inserts:\n{}",
format_btree(pager.clone(), root_page, 0)
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably be lower log level like trace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

for &(key, size, _) in sequence.iter() {
let key = OwnedValue::Integer(key);
let value = Record::new(vec![OwnedValue::Blob(Rc::new(vec![0; size]))]);
log::info!("Inserting key: {}", key);
Copy link
Collaborator

Choose a reason for hiding this comment

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

trace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

SQLite does floor division but clippy erroneously says its floor division.
Ignore clippy warning.
The previous version when tested with large blobs was breaking invariants.
Added large blobs to try and break delete.
@krishvishal krishvishal force-pushed the fix-clear-overflow-page branch from d786340 to a1143f7 Compare February 12, 2025 17:43
@krishvishal
Copy link
Contributor Author

krishvishal commented Feb 12, 2025

@pereman2 I've added the requested changes.


The reason for this PR:

When I have added the fuzz test it has triggered some asserts in the clear_overflow_pages function. The reason for the breakage is because previous I've used div_ceil as clippy suggested (may be its a bug) by this would give us +1 greater than what SQLite gives, so we end up thinking there's 1 more overflow page.

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