Re: Parallel safety for extern params

On Fri, Oct 13, 2017 at 1:19 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> After fixing this problem, when I ran the regression tests with
> force_parallel_mode = regress, I saw multiple other failures. All the
> failures boil down to two kinds of cases:
>
> 1. There was an assumption while extracting simple expressions that
> the target list of gather node can contain constants or Var's. Now,
> once the above patch allows extern params as parallel-safe, that
> assumption no longer holds true. We need to handle params as well.
> Attached patch fix_simple_expr_interaction_gather_v1.patch handles
> that case.

- * referencing the child node's output ... but setrefs.c might also have
- * copied a Const as-is.
+ * referencing the child node's output or a Param... but setrefs.c might
+ * also have copied a Const as-is.

I think the Param case should be mentioned after "... but" not before
- i.e. referencing the child node's output... but setrefs.c might also
have copied a Const or Param is-is.

> 2. We don't allow execution to use parallelism if the plan can be
> executed multiple times. This has been enforced in ExecutePlan, but
> it seems like that we miss to handle the case where we are already in
> parallel mode by the time we enforce that condition. So, what
> happens, as a result, is that it will allow to use parallelism when it
> shouldn't (when the same plan is executed multiple times) and lead to
> a crash. One way to fix is that we temporarily exit the parallel mode
> in such cases and reenter in the same state once the current execution
> is over. Attached patch fix_parallel_mode_nested_execution_v1.patch
> fixes this problem.

This seems completely unsafe. If somebody's already entered parallel
mode, they are counting on having the parallel-mode restrictions
enforced until they exit parallel mode. We can't just disable those
restrictions for a while in the middle and then put them back.

I think the bug is in ExecGather(Merge): it assumes that if we're in
parallel mode, it's OK to start workers. But actually, it shouldn't
do this unless ExecutePlan ended up with use_parallel_mode == true,
which isn't quite the same thing. I think we might need ExecutePlan
to set a flag in the estate that ExecGather(Merge) can check.

Thanks for working on this.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company