Re: Proposal: scan key push down to heap [WIP]

On Mon, Nov 28, 2016 at 4:30 AM, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> On Sat, Nov 19, 2016 at 6:48 PM, Dilip Kumar <dilipbalaut(at)gmail(dot)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