Re: Patch: Write Amplification Reduction Method (WARM)

From: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
To: Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)com>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Bruce Momjian <bruce(at)momjian(dot)us>, Jaime Casanova <jaime(dot)casanova(at)2ndquadrant(dot)com>, Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Patch: Write Amplification Reduction Method (WARM)
Date: 2017-03-14 01:47:46
Message-ID: 20170314014746.i43pqkjs6clxsh56@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> @@ -234,6 +236,21 @@ index_beginscan(Relation heapRelation,
> scan->heapRelation = heapRelation;
> scan->xs_snapshot = snapshot;
>
> + /*
> + * If the index supports recheck, make sure that index tuple is saved
> + * during index scans.
> + *
> + * XXX Ideally, we should look at all indexes on the table and check if
> + * WARM is at all supported on the base table. If WARM is not supported
> + * then we don't need to do any recheck. RelationGetIndexAttrBitmap() does
> + * do that and sets rd_supportswarm after looking at all indexes. But we
> + * don't know if the function was called earlier in the session when we're
> + * here. We can't call it now because there exists a risk of causing
> + * deadlock.
> + */
> + if (indexRelation->rd_amroutine->amrecheck)
> + scan->xs_want_itup = true;
> +
> return scan;
> }

I didn't like this comment very much. But it's not necessary: you have
already given relcache responsibility for setting rd_supportswarm. The
only problem seems to be that you set it in RelationGetIndexAttrBitmap
instead of RelationGetIndexList, but it's not clear to me why. I think
if the latter function is in charge, then we can trust the flag more
than the current situation. Let's set the value to false on relcache
entry build, for safety's sake.

I noticed that nbtinsert.c and nbtree.c have a bunch of new includes
that they don't actually need. Let's remove those. nbtutils.c does
need them because of btrecheck(). Speaking of which:

I have already commented about the executor involvement in btrecheck();
that doesn't seem good. I previously suggested to pass the EState down
from caller, but that's not a great idea either since you still need to
do the actual FormIndexDatum. I now think that a workable option would
be to compute the values/isnulls arrays so that btrecheck gets them
already computed. With that, this function would be no more of a
modularity violation that HeapSatisfiesHOTAndKey() itself.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Craig Ringer 2017-03-14 02:22:59 Re: [PATCH] Transaction traceability - txid_status(bigint)
Previous Message Haribabu Kommi 2017-03-14 01:34:12 Re: [REVIEW] macaddr 64 bit (EUI-64) datatype support