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

On Mon, Nov 28, 2016 at 8:25 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> 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.

I agree.

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.

Actually we want to call slot_getattr instead heap_getattr, because of
problem mentioned by Andres upthread and we also saw in test results.

Should we make a copy of HeapKeyTest lets say ExecKeyTest and keep it
under executor ?

ExecKeyTest will be same as HeapKeyTest but it will call slot_getattr
instead of heap_getattr.

>
> - 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.
>
Okay.
> - get_scankey_from_qual()'s prohibition on variable-width columns is
> presumably no longer necessary with this approach, right?

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


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com