Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM) - Mailing list pgsql-hackers

Reading 0001_track_root_lp_v9.patch again:

> +/*
> + * We use the same HEAP_LATEST_TUPLE flag to check if the tuple's t_ctid field
> + * contains the root line pointer. We can't use the same
> + * HeapTupleHeaderIsHeapLatest macro because that also checks for TID-equality
> + * to decide whether a tuple is at the of the chain
> + */
> +#define HeapTupleHeaderHasRootOffset(tup) \
> +( \
> +     ((tup)->t_infomask2 & HEAP_LATEST_TUPLE) != 0 \
> +)
>
> +#define HeapTupleHeaderGetRootOffset(tup) \
> +( \
> +     AssertMacro(((tup)->t_infomask2 & HEAP_LATEST_TUPLE) != 0), \
> +     ItemPointerGetOffsetNumber(&(tup)->t_ctid) \
> +)

Interesting stuff; it took me a bit to see why these macros are this
way.  I propose the following wording which I think is clearer:

  Return whether the tuple has a cached root offset.  We don't use
  HeapTupleHeaderIsHeapLatest because that one also considers the slow
  case of scanning the whole block.

Please flag the macros that have multiple evaluation hazards -- there
are a few of them.

> +/*
> + * Get TID of next tuple in the update chain. Caller should have checked that
> + * we are not already at the end of the chain because in that case t_ctid may
> + * actually store the root line pointer of the HOT chain whose member this
> + * tuple is.
> + */
> +#define HeapTupleHeaderGetNextTid(tup, next_ctid) \
> +do { \
> +     AssertMacro(!((tup)->t_infomask2 & HEAP_LATEST_TUPLE)); \
> +     ItemPointerCopy(&(tup)->t_ctid, (next_ctid)); \
> +} while (0)

Actually, I think this macro could just return the TID so that it can be
used as struct assignment, just like ItemPointerCopy does internally --
callers can do
        ctid = HeapTupleHeaderGetNextTid(tup);

The API of RelationPutHeapTuple appears a bit contorted, where
root_offnum is both input and output.  I think it's cleaner to have the
argument be the input, and have the output offset be the return value --
please check whether that simplifies things; for example I think this:

> +                     root_offnum = InvalidOffsetNumber;
> +                     RelationPutHeapTuple(relation, buffer, heaptup, false,
> +                                     &root_offnum);

becomes

        root_offnum = RelationPutHeapTuple(relation, buffer, heaptup, false,
                        InvalidOffsetNumber);

Many comments lack finishing periods in complete sentences, which looks
odd.  Please fix.