Re: Parallel INSERT (INTO ... SELECT ...)

On Mon, Jan 25, 2021 at 10:23 AM tsunakawa(dot)takay(at)fujitsu(dot)com <
tsunakawa(dot)takay(at)fujitsu(dot)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().
>

Thanks, I'll fix that in the comments.

>
> (8)
> + /*
> + * If the trigger type is RI_TRIGGER_FK, this indicates a
FK exists in
> + * the relation, and this would result in creation of new
CommandIds
> + * on insert/update/delete and this isn't supported in a
parallel
> + * worker (but is safe in the parallel leader).
> + */
> + trigtype = RI_FKey_trigger_type(trigger->tgfoid);
> + if (trigtype == RI_TRIGGER_FK)
> + {
> + if
(max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context))
> + return true;
> + }
>
> Here, RI_TRIGGER_FK should instead be RI_TRIGGER_PK, because
RI_TRIGGER_FK triggers do not generate command IDs. See RI_FKey_check()
which is called in RI_TRIGGER_FK case. In there, ri_PerformCheck() is
called with the detectNewRows argument set to false, which causes
CommandCounterIncrement() to not be called.
>

Hmmm, I'm not sure that you have read and interpreted the patch code
correctly.
The existence of a RI_TRIGGER_FK trigger indicates the table has a foreign
key, and an insert into such a table will generate a new commandId (so we
must avoid that, as we don't currently have the technology to support
sharing of new command IDs across the participants in the parallel
operation). This is what the code comment says, It does not say that such a
trigger generates a new command ID.

See Amit's updated comment here:
https://github.com/postgres/postgres/commit/0d32511eca5aec205cb6b609638ea67129ef6665

In addition, the 2nd patch has an explicit test case for this (testing
insert into a table that has a FK).

If you have a test case that breaks the existing patch, please let me know.

Regards,
Greg Nancarrow
Fujitsu Australia