> On 1 Feb 2020, at 20:37, Tom Lane <t...@sss.pgh.pa.us> wrote: > > Hamid Akhtar <hamid.akh...@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
