On Mon, Apr 17, 2023 at 11:04 PM Tom Lane <t...@sss.pgh.pa.us> wrote:
> Richard Guo <guofengli...@gmail.com> writes:
> > So now it seems that the breakage of regression tests is more severe
> > than being cosmetic. I wonder if we need to update the comments to
> > indicate the potential wrong results issue if we move the initPlans to
> > the Gather node.
>
> I wondered about that too, but how come neither of us saw non-cosmetic
> failures (ie, actual query output changes not just EXPLAIN changes)
> when we tried this? Maybe the case is somehow not exercised, but if
> so I'm more worried about adding regression tests than comments.
Sorry I forgot to mention that I did see query output changes after
moving the initPlans to the Gather node. First of all let me make sure
I was doing it in the right way. On the base of the patch, I was using
the diff as below
if (debug_parallel_query != DEBUG_PARALLEL_OFF &&
- top_plan->parallel_safe && top_plan->initPlan == NIL)
+ top_plan->parallel_safe)
{
Gather *gather = makeNode(Gather);
+ gather->plan.initPlan = top_plan->initPlan;
+ top_plan->initPlan = NIL;
+
gather->plan.targetlist = top_plan->targetlist;
And then I changed the default value of debug_parallel_query to
DEBUG_PARALLEL_REGRESS. And then I just ran 'make installcheck' and saw
the query output changes.
> I think actually that it does work beyond the EXPLAIN weirdness,
> because since e89a71fb4 the Gather machinery knows how to transmit
> the values of Params listed in Gather.initParam to workers, and that
> is filled in setrefs.c in a way that looks like it'd work regardless
> of whether the Gather appeared organically or was stuck on by the
> debug_parallel_query hackery. I've not tried to verify that
> directly though.
It seems that in this case the top_plan does not have any extParam, so
the Gather node that is added atop the top_plan does not have a chance
to get its initParam filled in set_param_references().
Thanks
Richard
