From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Pavan Deolasee <pavan.deolasee@gmail.com>
Cc: Jaime Casanova <jaime.casanova@2ndquadrant.com>
Cc: Haribabu Kommi <kommi.haribabu@gmail.com>
Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Patch: Write Amplification Reduction Method (WARM)
Date: Wed, 25 Jan 2017 13:36:48 -0300
Message-ID: <20170125163648.w3q24ifflvcemjpj@alvherre.pgsql> (raw)
In-Reply-To: <CABOikdNigDQ59DyAk1hQh6PpDJwaVqs3VV4ZqqFtDHZiz9-2-Q@mail.gmail.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-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.
> +/*
> + * If HEAP_LATEST_TUPLE is set in the last tuple in the update chain. But for
> + * clusters which are upgraded from pre-10.0 release, we still check if c_tid
> + * is pointing to itself and declare such tuple as the latest tuple in the
> + * chain
> + */
> +#define HeapTupleHeaderIsHeapLatest(tup, tid) \
> +( \
> + (((tup)->t_infomask2 & HEAP_LATEST_TUPLE) != 0) || \
> + ((ItemPointerGetBlockNumber(&(tup)->t_ctid) == ItemPointerGetBlockNumber(tid)) && \
> + (ItemPointerGetOffsetNumber(&(tup)->t_ctid) == ItemPointerGetOffsetNumber(tid))) \
> +)
I suggest rewording this comment as:
Starting from PostgreSQL 10, the latest tuple in an update chain has
HEAP_LATEST_TUPLE set; but tuples upgraded from earlier versions do
not. For those, we determine whether a tuple is latest by testing
that its t_ctid points to itself.
(as discussed, there is no "10.0 release"; it's called the "10 release"
only, no ".0". Feel free to use "v10" or "pg10").
> +/*
> + * 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);
or more precisely, this pattern
> + if (!HeapTupleHeaderIsHeapLatest(tp.t_data, &tp.t_self))
> + HeapTupleHeaderGetNextTid(tp.t_data, &hufd->ctid);
> + else
> + ItemPointerCopy(&tp.t_self, &hufd->ctid);
becomes
hufd->ctid = HeapTupleHeaderIsHeapLatest(foo) ?
HeapTupleHeaderGetNextTid(foo) : &tp->t_self;
or something like that. I further wonder if it'd make sense to hide
this into yet another macro.
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);
Please remove the words "must have" in this comment:
> + /*
> + * Also mark both copies as latest and set the root offset information. If
> + * we're doing a HOT/WARM update, then we just copy the information from
> + * old tuple, if available or computed above. For regular updates,
> + * RelationPutHeapTuple must have returned us the actual offset number
> + * where the new version was inserted and we store the same value since the
> + * update resulted in a new HOT-chain
> + */
Many comments lack finishing periods in complete sentences, which looks
odd. Please fix.
I have not looked at the other patch yet.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@2ndquadrant.com, pavan.deolasee@gmail.com, jaime.casanova@2ndquadrant.com, kommi.haribabu@gmail.com, tomas.vondra@2ndquadrant.com
Subject: Re: Patch: Write Amplification Reduction Method (WARM)
In-Reply-To: <20170125163648.w3q24ifflvcemjpj@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox