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

From Alvaro Herrera
Subject Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)
Date
Msg-id 20170314014746.i43pqkjs6clxsh56@alvherre.pgsql
Whole thread Raw
In response to Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)  (Alvaro Herrera <alvherre@2ndquadrant.com>)
Responses Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)  (Pavan Deolasee <pavan.deolasee@gmail.com>)
Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)  (Pavan Deolasee <pavan.deolasee@gmail.com>)
List 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



pgsql-hackers by date:

Previous
From: Haribabu Kommi
Date:
Subject: Re: [HACKERS][REVIEW] macaddr 64 bit (EUI-64) datatype support
Next
From: Craig Ringer
Date:
Subject: Re: [HACKERS] [PATCH] Transaction traceability - txid_status(bigint)