Page deltas code cleanup

    • Type: Task
    • Resolution: Unresolved
    • Priority: Major - P3
    • None
    • Affects Version/s: None
    • Component/s: Reconciliation
    • Storage Engines, Storage Engines - Persistence
    • None
    • None

      The review of page deltas (https://github.com/wiredtiger/wiredtiger/pull/12076) identified several issues:

      • The documentation string for the delta-related config options should say "reconciliation may write deltas" rather than "reconciliation will write deltas"
      • The ref_changes variable should be read using acquire read
      • The change in the disagg branch dropped the assertion in split_multi_inmem_final about restoring saved updated to the new page. We can tweak the assertion (see the PR for details) and reinstate it.
      • We could possibly optimise the number of flags used, see the PR for details.
      • In wt_txn_read_upd_list_internal, there's some new functionality to search the history store if we see an update that is restored from a delta and not visible to the reader. We don't need to perform this search if the dhandle is metadata.
      • In rec_row_garbage_collect_fixup_update_list, we can set the new tombstone's transaction id to WT_TXN_NONE instead of the previous update's transaction ID (see the PR for details.)
      • There's a likely bug in wti_rec_row_leaf. We can only garbage collect an update if it has no stop timestamp – there's a proposed fix on the PR.
      • Variables need to be alphabetised in several places.
      • There's a mistake in cell.h where we outline the structure of wt_delta_cell_leaf. It lists the key length twice, the second key length should be the value length.
      • The comment is wrong about deletes, there's a suggested fix on the PR.
      • The original change removed an assertion in rec_fill_tw_from_upd_select, where we check that a tombstone written to disk should have the full value. We can reinstate this assertion by first checking WT_BTREE_DISAGGREGATED, there is more information on the PR.
      • In rec_write_page_status, it's unclear where r->rec_start_pinned_stable_ts is set.
      • The comment in rec_split_write about needing to assign a new page ID for the root every time needs to be changed – we don't support deltas for root pages yet.
      • The if (build_delta) case in rec_split_write ought to be a separate function.
      • rec_split_discard unnecessarily assigns WT_BLOCK_INVALID_PAGE_ID to multi->block_meta.page_id.
      • In that same function, it's unclear whether we really need the mod->rec_result = 0 assignment.

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

              Created:
              Updated: