On Wed, Oct 14, 2015 at 3:29 AM, Robert Haas <robertmh...@gmail.com> wrote: > > On Tue, Oct 13, 2015 at 2:45 AM, Amit Kapila <amit.kapil...@gmail.com> wrote: > > Attached is rebased patch for partial seqscan support. > > Review comments: > > - If you're going to pgindent execParallel.c, you need to add some > entries to typedefs.list so it doesn't mangle the formatting. > ExecParallelEstimate's parameter list is misformatted, for example. > Also, I think if we're going to do this we had better extract the > pgindent changes and commit those first. It's pretty distracting the > way you have it. >
Agreed, we can do those separately. So, I have reverted those changes. > - Instead of inlining the work needed by each parallel mode in > ExecParallelEstimate(), I think you should mimic the style of > ExecProcNode and call a node-type specific function that is part of > that node's public interface - here, ExecPartialSeqScanEstimate, > perhaps. Similarly for ExecParallelInitializeDSM. Perhaps > ExecPartialSeqScanInitializeDSM. > > - I continue to think GetParallelShmToc is the wrong approach. > Instead, each time ExecParallelInitializeDSM or > ExecParallelInitializeDSM calls a nodetype-specific initialized > function (as described in the previous point), have it pass d->pcxt as > an argument. The node can get the toc from there if it needs it. I > suppose it could store a pointer to the toc in its scanstate if it > needs it, but it really shouldn't. Instead, it should store a pointer > to, say, the ParallelHeapScanDesc in the scanstate. It really should > only care about its own shared data, so once it finds that, the toc > shouldn't be needed any more. Then ExecPartialSeqScan doesn't need to > look up pscan; it's already recorded in the scanstate. > > - ExecParallelInitializeDSMContext's new pscan_len member is 100% > wrong. Individual scan nodes don't get to add stuff to that context > object. They should store details like this in their own ScanState as > needed. > Changed the patch as per above suggestions. > - The positioning of the new nodes in various lists doesn't seem to > entirely consistent. nodes.h adds them after SampleScan which isn't > terrible, though maybe immediately after SeqScan would be better, but > _outNode has it right after BitmapOr and the switch in _copyObject has > it somewhere else again. > I think this got messed up while rebasing on top of Gather node changes, but nonetheless, I have changed it such that PartialSeqScan node handling is after SeqScan. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
parallel_seqscan_partialseqscan_v21.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
