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

On Wed, Feb 24, 2021 at 10:38 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > 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.
>

Yes, but you're potentially now adding overhead to every
SELECT/UPDATE/DELETE with a subquery, by the added recursive checking
and walking done by the new call to max_parallel_hazard_walker().and
code block looking for a modifying CTE
And anyway I'm not sure it's really right putting in a fix for SELECT
with a modifying CTE, into a patch that adds parallel INSERT
functionality - in any case you'd need to really spell this out in
code comments, as this is at best a temporary fix that would need to
be removed whenever the query rewriter is fixed to set hasModifyingCTE
correctly.

> > - 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.
>

parse->hasModifyingCTE is not just used in the shortcut-test for
parallel-safety, its value is subsequently copied into the PlannedStmt
returned by standard_planner.
It's inconsistent to leave hasModifyingCTE FALSE when by the fix it
has found a modifying CTE.
Even if no existing tests detect an issue with this, PlannedStmt is
left with a bad hasModifyingCTE value in this case, so there is the
potential for something to go wrong.

> > - 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.
>

At least my current fix is very specific, efficient and clear in its
purpose, and suitably documented, so it is very clear when and how it
is to be removed, when the issue is fixed in the query rewriter.
Another concern with the alternative fix is that it always searches
for a modifying CTE, even when parse->hasModifyingCTE is true after
the query rewriter processing.

Regards,
Greg Nancarrow
Fujitsu Australia