On Mon, Nov 28, 2016 at 8:25 PM, Robert Haas <robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
