Rework fragile iteration state logic in __clayered_iterate_constituents()

XMLWordPrintableJSON

    • Type: Improvement
    • Resolution: Unresolved
    • Priority: Major - P3
    • None
    • Affects Version/s: None
    • Component/s: Layered Tables
    • None
    • Storage Engines - Foundations
    • 134.956
    • None
    • 8

      Common-concern follow-up from the review of PR #14124 (WT-17959). The refactor split __clayered_iterate_constituents() into focused helpers, but the underlying iteration-state logic (carried over from the pre-refactor code) remains complex and fragile: correctness depends on several implicit, cross-function invariants that are easy to break with an innocent-looking change. This ticket collects the concrete spots so the logic can be reworked as a whole rather than patched case by case.

      Fragile places identified during review

      1. Implicit "flag set implies both constituents positioned" contract in the prepare-conflict branch. The else if chain that skips _clayered_position_alternate() while blocked on an ingest prepare conflict relies on (a) the iteration flag being cleared by _clayered_update_state() on any snapshot/checkpoint change and (b) the exit contract that both constituents were left positioned. A candidate edge case: the stable constituent is opened for the first time mid-walk while the cursor is blocked on an ingest prepare conflict, and the blocked key is later evicted from ingest after the checkpoint pickup — the freshly opened stable cursor may never be positioned. Related to WT-17969. (discussion)
      2. Alternate-advance ordering under a prepare conflict. Normally the alternate is compared and advanced against the current cursor before the current moves. When the current was blocked on a prepared key and the conflict resolves, the comparison effectively happens against the current cursor after it moved. Review concluded the current behavior is likely correct (the tombstone-resolution case is caught later), but only via subtle multi-step reasoning — the ordering invariant is not enforced or documented in the code. (discussion)
      3. Walk-in-progress detection based on "any constituent is positioned". Any error path that forgets to reset() a constituent silently flips the walk between fresh-positioning and steady-state advance. Proposal: a dedicated __clayered_iterate_positioned() predicate based on the current cursor's WT_CURSTD_KEY_INT / btree ref, which requires setting current_cursor when blocked on a prepare conflict (a behavior change). (discussion)
      4. Undocumented "NOTFOUND impossible here" invariant. Neither the author nor reviewers could initially derive why WT_NOTFOUND cannot occur at that point; a comment was added in the PR, but the invariant should be made structural or asserted. (discussion)
      5. Single-constituent walks leave the other constituent positioned. When __clayered_enter() supplies only one constituent for the operation, the other keeps its stale position instead of being reset, which interacts badly with item 3. (discussion)
      6. Proposed simplification of _clayered_advance_positioned(). Unify the prepare-blocked and steady-state paths: reposition the alternate whenever the iteration flag is not set, and compare/advance the alternate only when the current is the ingest cursor. Known open question: a prepare conflict hit inside _clayered_position_alternate() itself (also unhandled in the current code). (discussion)

      Suggested outcome

      • Make the positioning contract explicit: assert (not just assume) the state each branch relies on.
      • Reduce the number of special-cased branches, evaluating the __clayered_advance_positioned() simplification above as a starting point.
      • Cover the edge cases from items 1, 2 and 5 with tests once the reworked logic makes them expressible.

            Assignee:
            [DO NOT USE] Backlog - Storage Engines Team
            Reporter:
            Ivan Kochin
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Created:
              Updated: