Re: Patch: Write Amplification Reduction Method (WARM)

On Wed, Jan 25, 2017 at 10:06 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:

> Reading 0001_track_root_lp_v9.patch again:
>
>
Thanks for the review.

> > +/*
> > + * 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.
>

Umm, not scanning the whole block, but HeapTupleHeaderIsHeapLatest compares
t_ctid with the passed in TID and returns true if those matches. To know if
root lp is cached, we only rely on the HEAP_LATEST_TUPLE flag. Though if
the flag is set, then it implies latest tuple too.

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

Can you please tell me an example? I must be missing something.

>
>
> > +/*
> > + * 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);
>
>
Yes, makes sense. Will fix.

>
>
> 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);
>
>
Make sense. Will fix.

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

Sorry, not sure where I picked that style from. I see that the existing
code has both styles, though I will add finishing periods because I like
that way too.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services