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

On Thu, Jan 21, 2021 at 7:30 PM Amit Kapila <amit.kapila16@gmail.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