On Fri, Sep 18, 2015 at 6:55 AM, Amit Kapila <amit.kapil...@gmail.com> wrote: >> + currentRelation = ExecOpenScanRelation(estate, >> + ((SeqScan *) >> node->ss.ps.plan)->scanrelid, >> + eflags); >> >> I can't see how this can possibly be remotely correct. The funnel >> node shouldn't be limited to scanning a baserel (cf. fdw_scan_tlist). >> > > This is mainly used for generating tuple descriptor and that tuple > descriptor will be used for forming scanslot, funnel node itself won't > do any scan. However, we can completely eliminate this InitFunnel() > function and use ExecAssignProjectionInfo() instead of > ExecAssignScanProjectionInfo() to form the projection info.
That sounds like a promising approach. >> + buffer_usage_worker = (BufferUsage *)(buffer_usage + (i * >> sizeof(BufferUsage))); >> >> Cast it to a BufferUsage * first. Then you can use &foo[i] to find >> the i'th element. > > Do you mean to say that the way code is written won't work? > Values of structure BufferUsage for each worker is copied into string > buffer_usage which I believe could be fetched in above way. I'm just complaining about the style. If bar is a char*, then these are all equivalent: foo = (Quux *) (bar + (i * sizeof(Quux)); foo = ((Quux *) bar) + i; foo = &((Quux *) bar)[i]; baz = (Quux *) bar; foo = &baz[i]; >> + /* >> + * Re-initialize the parallel context and workers to perform >> + * rescan of relation. We want to gracefully shutdown all the >> + * workers so that they should be able to propagate any error >> + * or other information to master backend before dying. >> + */ >> + FinishParallelSetupAndAccumStats(node); >> >> Somehow, this makes me feel like that function is badly named. > > I think here comment seems to be slightly misleading, shall we > change the comment as below: > > Destroy the parallel context to gracefully shutdown all the > workers so that they should be able to propagate any error > or other information to master backend before dying. Well, why does a function that destroys the parallel context have a name that starts with FinishParallelSetup? It sounds like it is tearing things down, not finishing setup. >> +#parallel_setup_cost = 0.0 # same scale as above >> +#define DEFAULT_PARALLEL_SETUP_COST 0.0 >> >> This value is probably a bit on the low side. > > How about keeping it as 10.0? Really? I would have guessed that the correct value was in the tens of thousands. -- 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
