Date: Wed, 25 Jan 2017 18:08:32 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Pavan Deolasee <pavan.deolasee@gmail.com>
Cc: Jaime Casanova <jaime.casanova@2ndquadrant.com>,
	Haribabu Kommi <kommi.haribabu@gmail.com>,
	Tomas Vondra <tomas.vondra@2ndquadrant.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Patch: Write Amplification Reduction Method (WARM)
Message-ID: <20170125210832.p7johdmfnqvd2ysn@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CABOikdNigDQ59DyAk1hQh6PpDJwaVqs3VV4ZqqFtDHZiz9-2-Q@mail.gmail.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

Looking at your 0002 patch now.  It no longer applies, but the conflicts
are trivial to fix.  Please rebase and resubmit.

I think the way WARM works has been pretty well hammered by now, other
than the CREATE INDEX CONCURRENTLY issues, so I'm looking at the code
from a maintainability point of view only.

I think we should have some test harness for WARM as part of the source
repository.  A test that runs for an hour hammering the machine to
highest possible load cannot be run in "make check", of course, but we
could have some specific Make target to run it manually.  We don't have
this for any other feature, but this looks like a decent place to start.
Maybe we should even do it before going any further.  The test code you
submitted looks OK to test the feature, but I'm not in love with it
enough to add it to the repo.  Maybe I will spend some time trying to
convert it to Perl using PostgresNode.


I think having the "recheck" index methods create an ExecutorState looks
out of place.  How difficult is it to pass the estate from the calling
code?

IMO heap_get_root_tuple_one should be called just heap_get_root_tuple().
That function and its plural sibling heap_get_root_tuples() should
indicate in their own comments what the expectations are regarding the
root_offsets output argument, rather than deferring to the comments in
the "internal" function, since they differ on that point; for the rest
of the invariants I think it makes sense to say "Also see the comment
for heap_get_root_tuples_internal".  I wonder if heap_get_root_tuple
should just return the ctid instead of assigning the value to a
passed-in pointer, i.e.
OffsetNumber
heap_get_root_tuple(Page page, OffsetNumber target_offnum)
{
	OffsetNumber	off;
	heap_get_root_tuples_internal(page, target_offnum, &off);
	return off;
}


The simple_heap_update + CatalogUpdateIndexes pattern is getting
obnoxious.  How about creating something like catalog_heap_update which
does both things at once, and stop bothering each callsite with the WARM
stuff?  In fact, given that CatalogUpdateIndexes is used in other
places, maybe we should leave its API alone and create another function,
so that we don't have to change the many places that only do
simple_heap_insert.  (Places like OperatorCreate which do either insert
or update could just move the index update call into each branch.)


I'm not real sure about the interface between index AM and executor,
namely IndexScanDesc->xs_tuple_recheck.  For example, this pattern:
		if (!scan->xs_recheck)
			scan->xs_tuple_recheck = false;
		else
			scan->xs_tuple_recheck = true;
can become simply
	scan->xs_tuple_recheck = scan->xs_recheck;
which looks odd.  I can't pinpoint exactly what's the problem, though.
I'll continue looking at this one.

I wonder if heap_hot_search_buffer() and heap_hot_search() should return
a tri-valued enum instead of boolean; that idea looks reasonable in
theory but callers have to do more work afterwards, so maybe not.

I think heap_hot_search() sometimes leaving the buffer pinned is
confusing.  Really, the whole idea of having heap_hot_search have a
buffer output argument is an important API change that should be better
thought.  Maybe it'd be better to return the buffer pinned always, and
the caller is always in charge of unpinning if not InvalidBuffer.  Or
perhaps we need a completely new function, given how different it is to
the original?  If you tried to document in the comment above
heap_hot_search how it works, you'd find that it's difficult to
describe, which'd be an indicator that it's not well considered.

-- 
Á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