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

Add the snapshot crate, which implements snapshotting at a low level #1340

Merged
merged 4 commits into from
Jun 5, 2024

Conversation

gefjon
Copy link
Contributor

@gefjon gefjon commented Jun 5, 2024

Description of Changes

  • Requires making BlobHash be Serialize and Deserialize. For arcane macro-ology reasons, this requires writing BlobHash::SIZE instead of Self::SIZE (it gets embedded in a visitor struct or something).
  • Requires adding two new operators to BlobStore.
  • Adds a return value to Page::save_content_hash, for convenience.
  • Impls DerefMut for Pages.
  • Scary change: adds Table::pages_mut. I think possibly this operator should be unsafe, since write access to the Pages allows an undisciplined caller to violate the Table's assumptions by corrupting a Page. It seems like an anti-pattern to mark a method unsafe on the grounds that misusing its return value can cause UB, but I don't see a plausible alternative without making most methods on Page unsafe. Open to feedback on this one!
  • Adds Table::iter_pages_with_hashes, which iterates over all pages, ensuring their hashes are present before yielding.

API and ABI breaking changes

N/a

Expected complexity level and risk

3 - see above comment re: safety of Table::pages_mut.

Testing

Tested manually in #1279 . Not convinced there's a useful way write tests for this in isolation i.e. before integrating.

- Requires making `BlobHash` be `Serialize` and `Deserialize`.
  For arcane macro-ology reasons, this requires writing `BlobHash::SIZE`
  instead of `Self::SIZE` (it gets embedded in a visitor struct or something).
- Requires adding two new operators to `BlobStore`.
- Adds a return value to `Page::save_content_hash`, for convenience.
- Impls `DerefMut` for `Pages`.
- **Scary change:** adds `Table::pages_mut`.
  I think possibly this operator should be `unsafe`,
  since write access to the `Pages` allows an undisciplined caller
  to violate the `Table`'s assumptions by corrupting a `Page`.
  It seems like an anti-pattern to mark a method `unsafe` on the grounds that
  misusing its return value can cause UB,
  but I don't see a plausible alternative
  without making most methods on `Page` unsafe.
  Open to feedback on this one!
@gefjon gefjon requested a review from Centril June 5, 2024 18:01
@gefjon
Copy link
Contributor Author

gefjon commented Jun 5, 2024

As an alternative to pages_mut, I could expose a method Table::iter_hashed_pages&mut self) -> impl Iterator<Item = &Page> which updates the hashes internally before yielding, but doesn't give mutable access to the Page or the Pages. I like that better, and will update after lunch.

crates/snapshot/src/lib.rs Show resolved Hide resolved
crates/snapshot/src/lib.rs Show resolved Hide resolved
crates/snapshot/src/lib.rs Outdated Show resolved Hide resolved
crates/snapshot/src/lib.rs Outdated Show resolved Hide resolved
crates/snapshot/src/lib.rs Outdated Show resolved Hide resolved
crates/table/src/table.rs Outdated Show resolved Hide resolved
crates/snapshot/src/lib.rs Show resolved Hide resolved
crates/snapshot/src/lib.rs Show resolved Hide resolved
crates/snapshot/src/lib.rs Outdated Show resolved Hide resolved
crates/snapshot/src/lib.rs Show resolved Hide resolved
crates/snapshot/src/lib.rs Outdated Show resolved Hide resolved
crates/snapshot/src/lib.rs Outdated Show resolved Hide resolved
crates/snapshot/src/lib.rs Outdated Show resolved Hide resolved
crates/snapshot/src/lib.rs Show resolved Hide resolved
crates/snapshot/src/lib.rs Outdated Show resolved Hide resolved
@gefjon gefjon enabled auto-merge June 5, 2024 21:52
@gefjon gefjon added this pull request to the merge queue Jun 5, 2024
Merged via the queue into master with commit 8c5f40d Jun 5, 2024
6 checks passed
@gefjon gefjon mentioned this pull request Jun 6, 2024
2 tasks
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