Re: Parallel Inserts in CREATE TABLE AS

On 05-01-2021 04:59, Bharath Rupireddy wrote:
> On Mon, Jan 4, 2021 at 7:02 PM Bharath Rupireddy
> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>>
>>> + if (IS_PARALLEL_CTAS_DEST(gstate->dest) &&
>>> + ((DR_intorel *) gstate->dest)->into->rel &&
>>> + ((DR_intorel *) gstate->dest)->into->rel->relname)
>>> why would rel and relname not be there? if no rows have been inserted?
>>> because it seems from the intorel_startup function that that would be
>>> set as soon as startup was done, which i assume (wrongly?) is always done?
>>
>> Actually, that into clause rel variable is always being set in the gram.y for CTAS, Create Materialized View and SELECT INTO (because qualified_name non-terminal is not optional). My bad. I just added it as a sanity check. Actually, it's not required.
>>
>> create_as_target:
>> qualified_name opt_column_list table_access_method_clause
>> OptWith OnCommitOption OptTableSpace
>> {
>> $$ = makeNode(IntoClause);
>> $$->rel = $1;
>> create_mv_target:
>> qualified_name opt_column_list table_access_method_clause opt_reloptions OptTableSpace
>> {
>> $$ = makeNode(IntoClause);
>> $$->rel = $1;
>> into_clause:
>> INTO OptTempTableName
>> {
>> $$ = makeNode(IntoClause);
>> $$->rel = $2;
>>
>> I will change the below code:
>> + if (GetParallelInsertCmdType(gstate->dest) ==
>> + PARALLEL_INSERT_CMD_CREATE_TABLE_AS &&
>> + ((DR_intorel *) gstate->dest)->into &&
>> + ((DR_intorel *) gstate->dest)->into->rel &&
>> + ((DR_intorel *) gstate->dest)->into->rel->relname)
>> + {
>>
>> to:
>> + if (GetParallelInsertCmdType(gstate->dest) ==
>> + PARALLEL_INSERT_CMD_CREATE_TABLE_AS)
>> + {
>>
>> I will update this in the next version of the patch set.
>
> Attaching v20 patch set that has above change in 0001 patch, note that
> 0002 to 0004 patches have no changes from v19. Please consider the v20
> patch set for further review.
>
>
> With Regards,
> Bharath Rupireddy.
> EnterpriseDB: http://www.enterprisedb.com
>

Hi,

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.

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.

Apart from these small things v20-0001 looks (very) good to me.

v20-0002:
will reply on the specific mail-thread about the state machine

v20-0003 and v20-0004:
looks good to me.

Kind regards,
Luc