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