> On 1 Feb 2020, at 20:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Hamid Akhtar <hamid.akhtar@gmail.com> writes:
>> I've reviewed and verified this patch and IMHO, this is ready to be committed.
>
> I took a look at this and I don't think it's really going in the right
> direction. ISTM the clear intent of this code was to attach the "Subplans
> Removed" item as a field of the parent [Merge]Append node, but the author
> forgot about the intermediate "Plans" array. So I think that, rather than
> doubling down on a mistake, we ought to move where the field is generated
> so that it *is* a field of the parent node.
Right, that makes sense; +1 on the attached 0001 patch.
> This does lead to some field
> order rearrangement in text mode, as per the regression test changes,
> but I think that's not a big deal. (A change can only happen if there
> are initplan(s) attached to the parent node.)
Does that prevent backpatching this, or are we Ok with EXPLAIN text output not
being stable across minors? AFAICT Pg::Explain still works fine with this
change, but mileage may vary for other parsers.
> 0002 attached isn't committable, because nobody would want the overhead
> in production, but it seems like a good trick to keep up our sleeves.
Thats a neat trick, I wonder if it would be worth maintaining a curated list of
these tricks in a README under src/test to help others avoid/reduce wheel
reinventing?
cheers ./daniel