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

move block checksum from FileBuffer to BlockManager #6033

Merged
merged 1 commit into from
Jan 31, 2023

Conversation

jkub
Copy link
Contributor

@jkub jkub commented Jan 30, 2023

Previously FileBuffer implements checksummed IO. However, different BlockManager implementations will want different Checksum implementations. It's much simpler to just move the checksum operations to the BlockManager. This then allowed me to devirtualize other functions, including ConstructManagedBuffer.

While doing this, I noticed that FileBuffer had some cruft. malloced_xx and internal_xx were always equivalent and thus redundant. I removed one.

Other nits:

  • There were some compiler warnings about integer truncation in swizzleable_pointer.cpp, which I fixed.
  • ReplayState was retrieving the catalog of the attached database in an odd way. (Which was actually causing a bug in some of my tests, due to a transaction not being active during attach.) Now it just calls db.GetCatalog().

@Tishj
Copy link
Contributor

Tishj commented Jan 30, 2023

This now takes the Catalog from the current AttachedDatabase, no longer the default database
I wonder if that has any unwanted effect in this case?

Copy link
Collaborator

@Mytherin Mytherin left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Looks good to me.

It has been a while - but from what I remember the three sizes were as follows:

  • malloced_size was the total (sector-size aligned) size
  • internal_size was the size requested for allocation (potentially not sector-size aligned)
  • size was the actual size of the data that could be written (minus the checksum header)

If the size requested for allocation is always sector-size aligned then malloced_size and internal_size are always identical and we can indeed get rid of the separate malloced_size. As we don't require sector size alignment anymore and if it is not required for you either then I am definitely in favor of merging the two and simplifying the code.

@Mytherin
Copy link
Collaborator

This now takes the Catalog from the current AttachedDatabase, no longer the default database
I wonder if that has any unwanted effect in this case?

Actually that probably fixes a bug.

@Mytherin Mytherin merged commit d25a77f into duckdb:master Jan 31, 2023
@Mytherin
Copy link
Collaborator

Thanks!

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.

None yet

3 participants