Route layered cursor value transfers through encode/decode wrappers - WT-18194 followup

XMLWordPrintableJSON

    • Storage Engines - Foundations
    • 125.293
    • None
    • None

      Follow-up to WT-17933, proposed by ivan.kochin@mongodb.com in the PR review
      (PR 14323 thread).

      Layered tombstone encoding is kept correct today by convention and by the dist/s_tombstone_encoding.py static check: every site that moves a value between a constituent cursor and the outside world must remember the encode/decode step. Ivan's idea is to make the conversion structurally inseparable from the transfer itself, so a new read or write path gets it for free:

      • __clayered_deleted_encode() gets exactly one caller: __clayered_put() - storing a value into a constituent becomes indivisible from encoding (modify rewrites included).
      • __clayered_deleted_decode() is called only from a value-promotion wrapper (surfacing a constituent value on the user cursor) and a modify-base wrapper, plus the drain conversion which keeps its own contract.
      • The cursor code never reads or writes a constituent value except through these named wrappers; the only stores outside __clayered_put() are the raw delete marker and the plain-modify fast path.

      This also simplifies s_tombstone_encoding.py: the reviewed caller inventories collapse to the wrappers.

      Ivan prototyped the full change (wrappers.patch attached to the PR thread). Notes for whoever picks this up:

      • Two behavioral points from the prototype deserve focused review and tests: namespace-path modify rewrites re-search instead of reusing the held cursor position and the encode scratch buffer's lifetime moves into __clayered_put().

      After this work is done, assess if tombstone encoding linters still bring enough value to keep them in the codebase: dist/s_tombstone_encoding.py, dist/test_tombstone_encoding.py. In fact, there should be enough justification to keep them.

      Original comment:

      I thought about how to make it harder for developers to forget the encode/decode step in future changes. The approach I ended up with is to couple the conversion with the read/write itself: every place that moves a value between a constituent cursor and the outside world now goes through a small set of wrappers that do the conversion internally. Anyone writing a new read or write path will naturally reach for these wrappers and get the encoding for free. It should also simplify s_tombstone_encoding.py, since the reviewed caller inventories collapse down to the wrappers.
      
      The resulting structure:
      
      - __clayered_deleted_encode() has exactly one caller: __clayered_put(). Storing a value into a constituent is now indivisible from encoding — modify rewrites also go through it, via a new WTI_CLAYERED_PUT_MODIFY_REWRITE op.
      - __clayered_deleted_decode() has two callers in the layered cursor logic: __clayered_promote_value() (surfacing a constituent value on the user cursor) and __clayered_modify_apply_decoded() (decoding the base value a modify applies against). The only other caller is __wt_clayered_ingest_to_stable_value(), the drain conversion, which keeps its own contract.
      
      So the cursor code never reads or writes a value except through these named wrappers. The only stores outside __clayered_put() are the raw tombstone delete marker and the plain modify() fast path (FIXME-WT-18057).
      
      Two behavioral notes:
      
      - A modify rewrite of an encoded-namespace value now re-searches instead of using the held cursor position, because __clayered_put() sets the key. This only affects the rare namespace paths; the common plain-modify path is untouched.
      - The encode scratch buffer moved into __clayered_put() and is freed before it returns. That is safe because a successful insert or update re-points the constituent's value at internal update memory, and a failed non-overwrite insert returns a cursor-local copy of the existing value.
      
      It's not a small change, so if you don't think it makes things better, I don't want to block this PR on it. Here is the full diff.
      
      [wrappers.patch](https://github.com/user-attachments/files/30579398/wrappers.patch)
      

            Assignee:
            Yury Ershov
            Reporter:
            Yury Ershov
            Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

              Created:
              Updated: