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

On Wed, Feb 24, 2021 at 4:30 PM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
>
> On Wed, Feb 24, 2021 at 8:39 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Fri, Feb 19, 2021 at 6:56 AM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
> > >
> > > Posting a new version of the patches, with the following updates:
> > >
> >
> > I am not happy with the below code changes, I think we need a better
> > way to deal with this.
> >
> > @@ -313,19 +314,35 @@ standard_planner(Query *parse, const char
> > *query_string, int cursorOptions,
> > glob->transientPlan = false;
> > glob->dependsOnRole = false;
> >
> > + if (IsModifySupportedInParallelMode(parse->commandType) &&
> > + !parse->hasModifyingCTE)
> > + {
> > + /*
> > + * FIXME
> > + * There is a known bug in the query rewriter: re-written queries with
> > + * a modifying CTE may not have the "hasModifyingCTE" flag set. When
> > + * that bug is fixed, this temporary fix must be removed.
> > + *
> > + * Note that here we've made a fix for this problem only for a
> > + * supported-in-parallel-mode table-modification statement (i.e.
> > + * INSERT), but this bug exists for SELECT too.
> > + */
> > + parse->hasModifyingCTE = query_has_modifying_cte(parse);
> > + }
> > +
> >
> > I understand that this is an existing bug but I am not happy with this
> > workaround. I feel it is better to check for modifyingCTE in
> > max_parallel_hazard_walker. See attached, this is atop
> > v18-0001-Enable-parallel-SELECT-for-INSERT-INTO-.-SELECT.
> >
>
> Thanks, I'll try it.
> I did, however, notice a few concerns with your suggested alternative fix:
> - It is not restricted to INSERT (as current fix is).
>

So what? The Select also has a similar problem.

> - It does not set parse->hasModifyingCTE (as current fix does), so the
> return value (PlannedStmt) from standard_planner() won't have
> hasModifyingCTE set correctly in the cases where the rewriter doesn't
> set it correctly (and I'm not sure what strange side effects ??? that
> might have).

Here end goal is not to set hasModifyingCTE but do let me know if you
see any problem or impact.

> - Although the invocation of max_parallel_hazard_walker() on a RTE
> subquery will "work" in finally locating your fix's added
> "CommonTableExpr" parallel-safety disabling block for commandType !=
> CMD_SELECT, it looks like it potentially results in checking and
> walking over a lot of other stuff within the subquery not related to
> CTEs. The current fix does a more specific and efficient search for a
> modifying CTE.
>

I find the current fix proposed by you quite ad-hoc and don't think we
can go that way.

--
With Regards,
Amit Kapila.