Re: Parallel safety for extern params

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sat, Oct 28, 2017 at 8:02 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> I think we need to make changes in exec_simple_recheck_plan to make
>> the behavior similar to HEAD. With the attached patch, all tests
>> passed with force_parallel_mode.

> Committed to REL_10_STABLE.

Sorry for resurrecting such an old thread, but I just noticed what
commit 682ce911f did in exec_save_simple_expr:

@@ -6588,8 +6588,8 @@ exec_save_simple_expr(PLpgSQL_expr *expr, CachedPlan *cplan)
* force_parallel_mode is on, the planner might've stuck a Gather node
* atop that. The simplest way to deal with this is to look through the
* Gather node. The Gather node's tlist would normally contain a Var
- * referencing the child node's output ... but setrefs.c might also have
- * copied a Const as-is.
+ * referencing the child node's output, but it could also be a Param, or
+ * it could be a Const that setrefs.c copied as-is.
*/
plan = stmt->planTree;
for (;;)
@@ -6616,9 +6616,9 @@ exec_save_simple_expr(PLpgSQL_expr *expr, CachedPlan *cplan)
/* If setrefs.c copied up a Const, no need to look further */
if (IsA(tle_expr, Const))
break;
- /* Otherwise, it better be an outer Var */
- Assert(IsA(tle_expr, Var));
- Assert(((Var *) tle_expr)->varno == OUTER_VAR);
+ /* Otherwise, it had better be a Param or an outer Var */
+ Assert(IsA(tle_expr, Param) || (IsA(tle_expr, Var) &&
+ ((Var *) tle_expr)->varno == OUTER_VAR));
/* Descend to the child node */
plan = plan->lefttree;
}

I think this is completely wrong and should be reverted. There
cannot be a Param there, and if there were it would not represent
a reference to the Gather's child.

The argument for this change seems to be what Amit said upthread:

>>> I am not sure if we can write the comment like that (.. copied a Const
>>> or Param as-is.) because fix_upper_expr_mutator in setrefs.c has a
>>> special handling for Var and Param where constants are copied as-is
>>> via expression_tree_mutator.

but AFAICS that is based on a misreading of what
fix_upper_expr_mutator does:

/* Special cases (apply only AFTER failing to match to lower tlist) */
if (IsA(node, Param))
return fix_param_node(context->root, (Param *) node);

Note the comment. A Param that matches something in the original
Result node would have been replaced by a Var reference to the
lower Result. If we find a Param still surviving in the Gather's
tlist, then it is not such a reference, and descending as though
it were is wrong and dangerous. AFAICS, the only case where we'd
actually find such a Param in a Gather is if it's a reference to
the value of an initplan --- but exec_save_simple_expr has already
asserted that there's no initPlans attached to the Gather.

I tried reverting this code change, and check-world still passes,
with or without debug_parallel_query = regress. So if there is
a case I'm missing, the regression tests don't expose it.

regards, tom lane