Re: Parallel INSERT (INTO ... SELECT ...) - Mailing list pgsql-hackers

On Mon, Jan 25, 2021 at 10:23 AM tsunakawa.takay@fujitsu.com <tsunakawa.takay@fujitsu.com> wrote:
>
> Hello Greg-san,
>
>
> Second group of comments (I'll reply to (1) - (4) later):
>
>
> (5)
> @@ -790,7 +790,8 @@ ExecCheckXactReadOnly(PlannedStmt *plannedstmt)
> ...
> -       if (plannedstmt->commandType != CMD_SELECT || plannedstmt->hasModifyingCTE)
> +       if ((plannedstmt->commandType != CMD_SELECT &&
> +                !IsModifySupportedInParallelMode(plannedstmt->commandType)) || plannedstmt->hasModifyingCTE)
>                 PreventCommandIfParallelMode(CreateCommandName((Node *) plannedstmt));
>  }
>
> Now that we're trying to allow parallel writes (INSERT), we should:
>
> * use ExecCheckXactReadOnly() solely for checking read-only transactions, as the function name represents.  That is, move the call to PreventCommandIfParallelMode() up to standard_ExecutorStart().
>
> * Update the comment  above the call to ExecCheckXactReadOnly().
>
>

Hmmm, I not so sure. The patch changes just make the existing test for calling PreventCommandIfParallelMode() a bit more restrictive, to exclude the Parallel INSERT case. So the code previously wasn't just checking read-only transactions anyway, so it's not as if the patch has changed something fundamental in this function. And by moving the PreventCommandIfParallelMode() call to a higher level, then you're making a change to the existing order of error-handling (as ExecCheckXactReadOnly() is calling PreventCommandIfReadOnly() based on a few other range-table conditions, prior to testing whether to call PreventCommandIfParallelMode()). I don't want to introduce a bug by making the change that you're suggesting.


> (6)
> @@ -764,6 +777,22 @@ ExecInitParallelPlan(PlanState *planstate, EState *estate,
> ...
> +       else
> +       {
> +               pei->processed_count = NULL;
> +       }
>
> The braces can be deleted.
>

Yes they can be deleted, and I guess I will, but for the record, I personally prefer the explicit brackets, even if just one line, because:
- if more code ever needs to be added to the else, you'll need to add brackets anyway (and newbies might add extra lines tabbed in, thinking it's part of the else block  ...).
- I think it looks better and slightly easier to read, especially when there's a mix of cases with multiple code lines and single code lines
Of course, these kind of things could be debated forever, but I don't think it's such a big deal.

>
> (7)
> @@ -1400,6 +1439,16 @@ ParallelQueryMain(dsm_segment *seg, shm_toc *toc)
>                                                                                  true);
>         queryDesc = ExecParallelGetQueryDesc(toc, receiver, instrument_options);
>
> +       Assert(queryDesc->operation == CMD_SELECT || IsModifySupportedInParallelMode(queryDesc->operation));
> +       if (IsModifySupportedInParallelMode(queryDesc->operation))
> +       {
> +               /*
> +                * Record that the CurrentCommandId is used, at the start of the
> +                * parallel operation.
> +                */
> +               SetCurrentCommandIdUsedForWorker();
> +       }
> +
>         /* Setting debug_query_string for individual workers */
>         debug_query_string = queryDesc->sourceText;
>
> @@ -765,12 +779,16 @@ GetCurrentCommandId(bool used)
>         if (used)
>         {
>                 /*
> -                * Forbid setting currentCommandIdUsed in a parallel worker, because
> -                * we have no provision for communicating this back to the leader.  We
> -                * could relax this restriction when currentCommandIdUsed was already
> -                * true at the start of the parallel operation.
> +                * If in a parallel worker, only allow setting currentCommandIdUsed if
> +                * currentCommandIdUsed was already true at the start of the parallel
> +                * operation (by way of SetCurrentCommandIdUsed()), otherwise forbid
> +                * setting currentCommandIdUsed because we have no provision for
> +                * communicating this back to the leader. Once currentCommandIdUsed is
> +                * set, the commandId used by leader and workers can't be changed,
> +                * because CommandCounterIncrement() then prevents any attempted
> +                * increment of the current commandId.
>                  */
> -               Assert(!IsParallelWorker());
> +               Assert(!(IsParallelWorker() && !currentCommandIdUsed));
>                 currentCommandIdUsed = true;
>         }
>         return currentCommandId;
>
> What happens without these changes?  

The change to the above comment explains why the change is needed.
Without these changes, a call in a parallel worker to GetCurrentCommandId() will result in an Assert being fired because (prior to the patch) currentCommandIdUsed is forbidden to be set in a parallel worker, and calling GetCurrentCommandId(true) (to signify the intent to use the returned CommandId to mark inserted/updated/deleted tuples) will result in currentCommandIdUsed being set to true.
So it is clear that this cannot remain the same, in order to support Parallel INSERT by workers.
So for each worker, the patch sets "currentCommandIdUsed" to true at the start of the parallel operation (using SetCurrentCommandIdUsedForWorker()) and the Assert condition in GetCurrentCommandId() is tweaked to fire the Assert if GetCurrentCommandId(true) is called in a parallel worker when currentCommandIdUsed is false;
To me, this makes perfect sense.


>If this kind of change is really necessary, it seems more natural to pass currentCommandIdUsed together with currentCommandId through SerializeTransactionState() and StartParallelWorkerTransaction(), instead of the above changes.

No, I don't agree with that. That approach doesn't sound right to me at all.
All the patch really changes is WHERE "currentCurrentIdUsed" can be set for a parallel worker - now it is only allowed to be set to true at the start of the parallel operation for each worker, and the Assert (which is just a sanity check) is updated to ensure that for workers, it can only be set true at that time. That's all it does. It's completely consistent with the old comment that said "We could relax this restriction when currentCommandIdUsed was already true at the start of the parallel operation" - that's what we are now doing with the patch.


> As an aside, SetCurrentCommandIdUsed() in the comment should be SetCurrentCommandIdUsedForWorker().
>