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

On Fri, Jan 22, 2021 at 8:29 AM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
>
> 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?
>

How it behaves when the table in the above test is a non-temp table
with your patch? If it leads to the same error then we can at least
conclude that this is a generic problem and nothing specific to temp
tables.

--
With Regards,
Amit Kapila.