Add a combined WT_IS_ANY_METADATA() macro for handles that are either metadata tree

XMLWordPrintableJSON

    • Type: Technical Debt
    • Resolution: Fixed
    • Priority: Minor - P4
    • WT12.0.0, 9.1.0-rc0
    • Affects Version/s: None
    • Component/s: Metadata
    • None
    • Storage Engines, Storage Engines - Persistence, Storage Engines - Transactions
    • 0.004
    • None
    • None

      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.

            Assignee:
            Etienne Petrel
            Reporter:
            Etienne Petrel
            Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

              Created:
              Updated:
              Resolved: