Re: Parallel Seq Scan

On Fri, Sep 18, 2015 at 6:55 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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