On Tue, Jan 5, 2021 at 1:00 PM Luc Vlaming <luc@swarm64.com> wrote:
> Reviewing further v20-0001:
>
> I would still opt for moving the code for the parallel worker into a
> separate function, and then setting rStartup of the dest receiver to
> that function in ExecParallelGetInsReceiver, as its completely
> independent code. Just a matter of style I guess.
If we were to have a intorel_startup_worker and assign it to
self->pub.rStartup, 1) we can do it in the CreateIntoRelDestReceiver,
we have to pass a parameter to CreateIntoRelDestReceiver as an
indication of parallel worker, which requires code changes in places
wherever CreateIntoRelDestReceiver is used. 2) we can also assign
intorel_startup_worker after CreateIntoRelDestReceiver in
ExecParallelGetInsReceiver, but that doesn't look good to me. 3) we
can duplicate CreateIntoRelDestReceiver and have a
CreateIntoRelParallelDestReceiver with the only change being that
self->pub.rStartup = intorel_startup_worker;
IMHO, the way it is currently, looks good. Anyways, I'm open to
changing that if we agree on any of the above 3 ways.
If we were to do any of the above, then we might have to do the same
thing for other commands Refresh Materialized View or Copy To where we
can parallelize.
Thoughts?
> Maybe I'm not completely following why but afaics we want parallel
> inserts in various scenarios, not just CTAS? I'm asking because code like
> + if (fpes->ins_cmd_type == PARALLEL_INSERT_CMD_CREATE_TABLE_AS)
> + pg_atomic_add_fetch_u64(&fpes->processed,
> queryDesc->estate->es_processed);
> seems very specific to CTAS. For now that seems fine but I suppose that
> would be generalized soon after? Basically I would have expected the if
> to compare against PARALLEL_INSERT_CMD_UNDEF.
After this patch is reviewed and goes for commit, then the next thing
I plan to do is to allow parallel inserts in Refresh Materialized View
and it can be used for that. I think the processed variable can also
be used for parallel inserts in INSERT INTO SELECT [1] as well.
Currently, I'm keeping it for CTAS, maybe later (after this is
committed) it can be generalized.
Thoughts?
[1] - https://www.postgresql.org/message-id/CAA4eK1LMmz58ej5BgVLJ8VsUGd%3D%2BKcaA8X%3DkStORhxpfpODOxg%40mail.gmail.com
> Apart from these small things v20-0001 looks (very) good to me.
> v20-0003 and v20-0004:
> looks good to me.
Thanks.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
