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