On Fri, Oct 13, 2017 at 1:19 AM, Amit Kapila <amit.kapil...@gmail.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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to