On Mon, Nov 28, 2016 at 4:30 AM, Dilip Kumar <dilipbal...@gmail.com> wrote:
> On Sat, Nov 19, 2016 at 6:48 PM, Dilip Kumar <dilipbal...@gmail.com> wrote:
>> patch1: Original patch (heap_scankey_pushdown_v1.patch), only
>> supported for fixed length datatype and use heap_getattr.
>> patch2: Switches memory context in HeapKeyTest + Store tuple in slot
>> and use slot_getattr instead of heap_getattr.
>> patch3: call HeapKeyTest in SeqNext after storing slot, and also
>> switch memory context before calling HeapKeyTest.
>> I haven't yet tested patch3 with TPCH, I will do that once machine is 
>> available.
> As promised, I have taken the performance with TPCH benchmark and
> still result are quite good. However this are less compared to older
> version (which was exposing expr ctx and slot to heap).
> Query       Head             [1] Patch3               Improvement
> Q3           36122.425      32285.608         10%
> Q4           6797               5763.871           15%
> Q10        17996.104      15878.505          11%
> Q12        12399.651        9969.489          19%
>  [1] heap_scankey_pushdown_POC_V3.patch : attached with the mail.

I think we should go with this approach.  I don't think it's a good
idea to have the heapam layer know about executor slots.  Even though
it's a little sad to pass up an opportunity for a larger performance
improvement, this improvement is still quite good.  However, there's a
fair amount of this patch that doesn't look right:

- The changes to heapam.c shouldn't be needed any more.  Ditto
valid.h, relscan.h, catcache.c and maybe some other stuff.

- get_scankey_from_qual() should be done at plan time, not execution
time.  Just as index scans already divide up quals between "Index
Cond" and "Filter" (see ExplainNode), I think Seq Scans are now going
to need to something similar.  Obviously, "Index Cond" isn't an
appropriate name for things that we test via HeapKeyTest, but maybe
"Heap Cond" would be suitable. That's going to be a fair amount of
refactoring, since the "typedef Scan SeqScan" in plannodes.h is going
to need to be replaced by an actual new structure definition.

- get_scankey_from_qual()'s prohibition on variable-width columns is
presumably no longer necessary with this approach, right?

- Anything tested in SeqNext() will also need to be retested in
SeqRecheck(); otherwise, the test will be erroneously skipped during
EPQ rechecks.

Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to