Issue Summary
Thirteen call sites test WT_IS_METADATA() and WT_IS_DISAGG_META() together to mean "either metadata tree", but no combined predicate exists for data handles, so every site spells the pair out by hand. Adding one macro removes the duplication and, more importantly, removes the failure mode where a new check covers the metadata file and silently forgets the disaggregated shared metadata.
That is exactly what [WT-18049] was: app_thread in __evict_snapshot_setup() excluded WT_IS_METADATA() but not WT_IS_DISAGG_META(), so an application thread read under a snapshot when evicting file:WiredTigerShared.wt_stable and discarded the checkpoint's uncommitted updates.
Context
A combined predicate already exists one level up, for URIs, in src/include/meta.h:
#define WT\_IS\_URI\_METADATA\(uri\)
\(strcmp\(\(uri\), WT\_METAFILE\_URI\) == 0 || strcmp\(\(uri\), WT\_DISAGG\_METADATA\_URI\) == 0\)
The data handle level is the gap. Both flags live in dhandle->flags (WT_DHANDLE_DISAGG_META 0x002u and WT_DHANDLE_IS_METADATA 0x080u in src/include/dhandle.h), and F_ISSET() is an any-of-mask test, so the combined check collapses to a single flag test.
The thirteen paired sites (line numbers as of the WT-18049 branch):
- src/evict/evict_page.c:1250 -- __evict_ckpt_snapshot_usable()
- src/evict/evict_page.c:1446 -- __evict_snapshot_setup(), the WT-18049 fix
- src/evict/evict_page.c:1526 -- __evict_reconcile()
- src/evict/evict_page.c:1594 -- __evict_reconcile()
- src/evict/evict_file.c:92 -- __wt_evict_file()
- src/reconcile/rec_write.c:619 -- __rec_write_page_status()
- src/reconcile/rec_write.c:832 -- __rec_init()
- src/reconcile/rec_write.c:3543 -- __rec_hs_wrapup()
- src/reconcile/rec_visibility.c:785 -- __rec_upd_select()
- src/include/btree_inline.h:1041 -- __wt_page_only_modify_set()
- src/include/btree_inline.h:1134 -- __wt_tree_modify_set()
- src/btree/bt_sync.c:98 -- __sync_scrub_checkpoint_enabled()
- src/btree/bt_sync.c:424 -- __wt_sync_file()
Two sites are deliberately disaggregated-only and must stay narrow:
- src/btree/bt_sync.c:160 -- __sync_checkpoint_can_skip()
- src/checkpoint/checkpoint_txn.c:517 -- *wt_checkpoint_get_handles(), inside an *wt_conn_is_disagg() branch
Proposed Solution
Add to src/include/meta.h, next to the existing handle predicates:
#define WT\_IS\_ANY\_METADATA\(dh\) F\_ISSET\(\(dh\), WT\_DHANDLE\_DISAGG\_META | WT\_DHANDLE\_IS\_METADATA\)
Then convert the thirteen paired sites, and drop the now-redundant "the metadata file or the disagg shared metadata file" longhand from the comments at those sites.
Out of scope:
- The other 45 WT_IS_METADATA() uses. Only the exact pairs convert; every other site needs a per-site judgement on whether the shared metadata belongs, which is not this ticket.
- The three sites that also test WT_IS_HS() in the !HS && !meta && !disagg_meta form (*wt_evict_file(), *rec_hs_wrapup(), __wt_sync_file()) arguably want their own "this tree may write to the history store" predicate. Real, but a separate change -- folding it in here makes the diff hard to review.
- Renaming WT_IS_METADATA() to something that reads as narrower. It would make the distinction obvious at every one of its 58 uses, but the churn is not worth it.
Definition of Done
- WT_IS_ANY_METADATA() defined in src/include/meta.h.
- All thirteen paired sites converted; no behaviour change intended anywhere.
- The two disaggregated-only sites left untouched.
- dist/s_all clean, and the Python suite and Catch2 tests pass -- a pure refactor, so any failure is a real mistake.
- is related to
-
WT-18379 Places that check the metadata file but miss the disaggregated shared metadata
-
- Closed
-
- related to
-
WT-15591 Review the code to ensure we also check the disagg shared metadata along with the local metadata
-
- Closed
-
-
WT-18379 Places that check the metadata file but miss the disaggregated shared metadata
-
- Closed
-
-
WT-18049 test_bug010.test_checkpoint_dirty: timestamp usage check aborts during checkpoint on WiredTigerShared.wt_stable
-
- Closed
-
- links to