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: http://www.postgresql.org/mailpref/pgsql-hackers
