Re: Parallel safety for extern params - Mailing list pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:
> On Sat, Oct 28, 2017 at 8:02 PM, Amit Kapila <amit.kapila16@gmail.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