Re: Parallel safety for extern params

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Mar 21, 2025 at 11:52 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> 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.

> Did you try the test case from the original post on this thread?

Yeah. It's fine, which is unsurprising even if I'm wrong, because
that test case has no involvement with plpgsql.

> I'm perfectly willing to believe that we messed up here -- this was 8
> years ago and I don't remember the details -- but it surprises me a
> little bit that I would have committed that change without verifying
> that it was necessary to resolve the reported problem.

Amit says in the original post that the regression tests failed
under force_parallel_mode = regress without this hack; but that's
demonstrably not true now. I spent a bit of quality time with
"git bisect" and found that the failures stopped at

0f7ec8d9c3aeb8964d3539561e5c8d4caef42bf6 is the first new commit
commit 0f7ec8d9c3aeb8964d3539561e5c8d4caef42bf6
Author: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Date: Wed Dec 12 13:49:41 2018 -0500

Repair bogus handling of multi-assignment Params in upper plan levels.

which appears unrelated if you just read the commit message, but the
actual diff tells the tale:

@@ -2325,8 +2325,6 @@ fix_join_expr_mutator(Node *node, fix_join_expr_context *context)
/* If not supplied by input plans, evaluate the contained expr */
return fix_join_expr_mutator((Node *) phv->phexpr, context);
}
- if (IsA(node, Param))
- return fix_param_node(context->root, (Param *) node);
/* Try matching more complex expressions too, if tlists have any */
if (context->outer_itlist && context->outer_itlist->has_non_vars)
{
@@ -2344,6 +2342,9 @@ fix_join_expr_mutator(Node *node, fix_join_expr_context *context)
if (newvar)
return (Node *) newvar;
}
+ /* Special cases (apply only AFTER failing to match to lower tlist) */
+ if (IsA(node, Param))
+ return fix_param_node(context->root, (Param *) node);
fix_expr_common(context->root, node);
return expression_tree_mutator(node,
fix_join_expr_mutator,

(and similarly in fix_upper_expr_mutator). So actually, I had broken
setrefs.c's matching of Params to lower plan levels with the
multi-assignment business, and Amit was dodging that breakage.
But this change is still wrong in itself: if anything, it should
have returned the Param, not treated it as a reference to the
child plan.

regards, tom lane