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

On Mon, Dec 7, 2020 at 2:35 PM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
>
> On Fri, Nov 20, 2020 at 7:44 PM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
> >
> > Posting an updated set of patches, with some additional testing and
> > documentation updates, and including the latest version of the
> > Parallel Insert patch.
> > Any feedback appreciated, especially on the two points mentioned in
> > the previous post.
> >
>
> Posting an updated set of patches, since a minor bug was found in the
> 1st patch that was causing a postgresql-cfbot build failure.
>

Most of the code present in
v9-0001-Enable-parallel-SELECT-for-INSERT-INTO-.-SELECT.patch is
applicable for parallel copy patch also. The patch in this thread
handles the check for PROPARALLEL_UNSAFE, we could slightly make it
generic by handling like the comments below, that way this parallel
safety checks can be used based on the value set in
max_parallel_hazard_context. There is nothing wrong with the changes,
I'm providing these comments so that this patch can be generalized for
parallel checks and the same can also be used by parallel copy.
Few comments:
1)
+ trigtype = RI_FKey_trigger_type(trigger->tgfoid);
+ if (trigtype == RI_TRIGGER_FK)
+ {
+ context->max_hazard = PROPARALLEL_RESTRICTED;
+
+ /*
+ * As we're looking for the max parallel
hazard, we don't break
+ * here; examine any further triggers ...
+ */
+ }

Can we change this something like:
trigtype = RI_FKey_trigger_type(trigger->tgfoid);
if (trigtype == RI_TRIGGER_FK)
{
if(max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context)
break;
}

This below line is not required as it will be taken care by
max_parallel_hazard_test.
context->max_hazard = PROPARALLEL_RESTRICTED;

2)
+ /* Recursively check each partition ... */
+ pdesc = RelationGetPartitionDesc(rel);
+ for (i = 0; i < pdesc->nparts; i++)
+ {
+ if (rel_max_parallel_hazard_for_modify(pdesc->oids[i],
+
command_type,
+
context,
+
AccessShareLock) == PROPARALLEL_UNSAFE)
+ {
+ table_close(rel, lockmode);
+ return context->max_hazard;
+ }
+ }

Can we change this something like:
/* Recursively check each partition ... */
pdesc = RelationGetPartitionDesc(rel);
for (i = 0; i < pdesc->nparts; i++)
{
char max_hazard = rel_max_parallel_hazard_for_modify(pdesc->oids[i],

command_type,

context,

AccessShareLock);

if(max_parallel_hazard_test(max_hazard, context)
{
table_close(rel, lockmode);
return context->max_hazard;
}
}

3)
Similarly for the below:
+ /*
+ * If there are any index expressions, check that they are parallel-mode
+ * safe.
+ */
+ if (index_expr_max_parallel_hazard_for_modify(rel, context) ==
PROPARALLEL_UNSAFE)
+ {
+ table_close(rel, lockmode);
+ return context->max_hazard;
+ }
+
+ /*
+ * If any triggers exist, check that they are parallel safe.
+ */
+ if (rel->trigdesc != NULL &&
+ trigger_max_parallel_hazard_for_modify(rel->trigdesc,
context) == PROPARALLEL_UNSAFE)
+ {
+ table_close(rel, lockmode);
+ return context->max_hazard;
+ }

4) Similar change required for the below:
+ /*
+ * If the column is of a DOMAIN type,
determine whether that
+ * domain has any CHECK expressions that are
not parallel-mode
+ * safe.
+ */
+ if (get_typtype(att->atttypid) == TYPTYPE_DOMAIN)
+ {
+ if
(domain_max_parallel_hazard_for_modify(att->atttypid, context) ==
PROPARALLEL_UNSAFE)
+ {
+ table_close(rel, lockmode);
+ return context->max_hazard;
+ }
+ }

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com