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

On Thu, Jan 21, 2021 at 7:30 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> > i.e. code-wise:
> >
> > /*
> > - * We can't support table modification in parallel-mode if
> > it's a foreign
> > - * table/partition (no FDW API for supporting parallel access) or a
> > + * We can't support table modification in a parallel worker if it's a
> > + * foreign table/partition (no FDW API for supporting parallel
> > access) or a
> > * temporary table.
> > */
> > if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE ||
> > RelationUsesLocalBuffers(rel))
> > {
> > - table_close(rel, lockmode);
> > - context->max_hazard = PROPARALLEL_UNSAFE;
> > - return true;
> > + if (max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context))
> > + {
> > + table_close(rel, lockmode);
> > + return true;
> > + }
> > }
> >
>
> Yeah, these changes look correct to me.
>

Unfortunately, this change results in a single test failure in the
"with" tests when "force_parallel_mode=regress" is in effect.

I have reproduced the problem, by extracting relevant SQL from those
tests, as follows:

CREATE TEMP TABLE bug6051 AS
select i from generate_series(1,3) as i;
SELECT * FROM bug6051;
CREATE TEMP TABLE bug6051_2 (i int);
CREATE RULE bug6051_ins AS ON INSERT TO bug6051 DO INSTEAD
INSERT INTO bug6051_2
SELECT NEW.i;
WITH t1 AS ( DELETE FROM bug6051 RETURNING * )
INSERT INTO bug6051 SELECT * FROM t1;
ERROR: cannot delete tuples during a parallel operation

Note that prior to the patch, all INSERTs were regarded as
PARALLEL_UNSAFE, so this problem obviously didn't occur.
I believe this INSERT should be regarded as PARALLEL_UNSAFE, because
it contains a modifying CTE.
However, for some reason, the INSERT is not regarded as having a
modifying CTE, so instead of finding it PARALLEL_UNSAFE, it falls into
the parallel-safety-checks and is found to be PARALLEL_RESTRICTED:

The relevant code in standard_planner() is:

if ((cursorOptions & CURSOR_OPT_PARALLEL_OK) != 0 &&
IsUnderPostmaster &&
(parse->commandType == CMD_SELECT ||
IsModifySupportedInParallelMode(parse->commandType)) &&
!parse->hasModifyingCTE &&
max_parallel_workers_per_gather > 0 &&
!IsParallelWorker())
{
/* all the cheap tests pass, so scan the query tree */
glob->maxParallelHazard = max_parallel_hazard(parse);
glob->parallelModeOK = (glob->maxParallelHazard != PROPARALLEL_UNSAFE);
}
else
{
/* skip the query tree scan, just assume it's unsafe */
glob->maxParallelHazard = PROPARALLEL_UNSAFE;
glob->parallelModeOK = false;
}

When I debugged this (transformWithClause()), the WITH clause was
found to contain a modifying CTE and for the INSERT
query->hasModifyingCTE was set true.
But somehow in the re-writer code, this got lost.
Bug?
Ideas?

Regards,
Greg Nancarrow
Fujitsu Australia