On Mon, Apr 17, 2023 at 11:04 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Richard Guo <guofenglinux@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
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().
