Remove the broken read-only get() accessors behind experimental-api-5 - #1343
Conversation
431d7fb to
8d5a8b0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1343 +/- ##
==========================================
- Coverage 91.02% 91.02% -0.01%
==========================================
Files 37 37
Lines 18146 18220 +74
==========================================
+ Hits 16518 16584 +66
- Misses 1628 1636 +8
🚀 New features to boost your workflow:
|
cberner
left a comment
cberner
left a comment
There was a problem hiding this comment.
Apply my comments to all the other locations too. Also, don't we need to remove the broken range() method?
| /// | ||
| /// This method does not exist when the `experimental-api-5` feature is enabled, and will be | ||
| /// removed in redb 5. Use [`ReadableMultimapTable::get()`] or [`Self::get_owned()`] instead. |
| let table = read.open_table(TABLE).unwrap(); | ||
| assert_eq!(table.len().unwrap(), 5); | ||
| assert!(table.get(&999u64).unwrap().is_none()); | ||
| assert!(table.get_owned(&999u64).unwrap().is_none()); |
There was a problem hiding this comment.
A lot of these don't need to be converted -- they can just use the non-'static version
8d5a8b0 to
335a211
Compare
cberner
changed the title
|
Applied both comments to all locations in 335a211: the doc notes are gone from both On Generated by Claude Code |
| let read_txn = db.begin_read().unwrap(); | ||
| let table = read_txn.open_table(table_def).unwrap(); | ||
| let retrieved = table.get(1).unwrap().unwrap(); | ||
| let retrieved = ReadableTable::get(&table, 1).unwrap().unwrap(); |
There was a problem hiding this comment.
Why is this explicit syntax necessary?? I don't like it
There was a problem hiding this comment.
It isn't, dropped in 4a0d673 — plain table.get(...) everywhere now. The reason it existed: redb-derive can't cfg on redb's features, and on default features the inherent 'static get() shadows the trait method, leaving a plain use redb::ReadableTable unused (which -Dwarnings rejects). The import now carries #[allow(unused_imports)] with a comment instead, and the call sites stay untouched.
Generated by Claude Code
335a211 to
4a0d673
Compare
| // With experimental-api-5 the inherent 'static get() does not exist, so calls resolve to the | ||
| // trait method |
There was a problem hiding this comment.
Remove this. And in the other imports too
The inherent ReadOnlyTable::get() and ReadOnlyMultimapTable::get() return 'static guards and iterators that, contrary to their documentation, do not keep the read transaction alive. Holding one after dropping the ReadTransaction lets a concurrent writer reclaim the pages it references, which panics the writer's commit() on a debug assertion. redb 5 will drop them in favor of the Readable* trait accessors (no cost, lifetime-bound) and the get_owned() variants (reference counted, keep the transaction alive). Preview that removal under the experimental-api-5 feature, like the KeyRange change already does for the inherent range() methods: the two get() methods do not exist when it is enabled, and calls resolve to the trait accessor instead. get_owned() previously delegated to the removed methods, so route it through the tree and a private get_inner() helper instead. Tests that read through the inherent methods incidentally keep their get() calls, resolving to the non-'static trait accessor under the feature; get_arc_lifetime, which exercises the removed behavior itself, switches to get_owned() under the feature like range_arc already does. Assisted-by: Claude Code <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YCYPuM4nb94pF36ZX7R8sa
4a0d673 to
231368b
Compare
1 participant
Follow-up to #1306; companion to #1307, which adds deprecation notices for default-feature builds.
Change
The inherent
ReadOnlyTable::get()andReadOnlyMultimapTable::get()return'staticguards and iterators that, contrary to their documentation, do not keep the read transaction alive: holding one after dropping theReadTransactionlets a concurrent writer reclaim the referenced pages, which panics the writer'scommit()on a debug assertion.redb 5 will drop them in favor of the
Readable*trait accessors (zero cost, lifetime-bound) and the_owned()variants from #1306. This PR previews that removal underexperimental-api-5, the same way theKeyRangechange already removed the inherentrange()methods under the flag: the twoget()methods do not exist when it is enabled, and calls resolve to the trait accessor instead.Supporting changes:
get_owned()previously delegated to the removed methods; it now goes through the tree directly / a privateget_inner()helper, matching howrange_ownedroutes through the privaterange_in_bounds().get()calls, which resolve to the non-'statictrait accessor under the feature. Files that didn't haveReadableTablein scope gain a cfg-gated import; the redb-derive tests, which cannot cfg on redb's features, import it under#[allow(unused_imports)]since the inherent method shadows it on default features.get_arc_lifetime, which exercises the removed behavior itself, switches toget_owned()under the feature likerange_arcalready does.--all-features, CI no longer compiles or runs the two methods after this change; they remain in default-feature builds until redb 5.Testing
cargo test --all --all-featurespasses; the arc-lifetime tests also pass under default features--all-features), fmt, andcargo docare clean under--deny warnings🤖 Generated with Claude Code
https://claude.ai/code/session_01YCYPuM4nb94pF36ZX7R8sa