On 26 November 2017 at 22:56, Tom Lane <t...@sss.pgh.pa.us> wrote: > Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> writes: >> [ fix-rewrite-tlist-v4.patch ] > > I started reviewing this patch. I did not much like the fact that it > effectively moved rewriteTargetListUD to a different file and renamed it. > That seems like unnecessary code churn, plus it breaks the analogy with > rewriteTargetListIU, plus it will make back-patching harder (since that > code isn't exactly the same in back branches). I see little reason why > we can't leave it where it is and just make it non-static. It's not like > there's no other parts of the rewriter that the planner calls. >
I wonder if, years from now, it might look a bit odd that rewriteTargetListUD() is doing part of work of preptlist.c, is only called from there, and yet is located in the rewriter. Aside from having a similar name to rewriteTargetListIU(), what rewriteTargetListUD() does seems more like what preprocess_targetlist() does for rowmarks. The fact that rewriteTargetListIU() intentionally only applies to the parent, whereas preprocess_targetlist() and now rewriteTargetListUD() apply to each child, further destroys any similarity between rewriteTargetListUD() and rewriteTargetListIU(). The point about back-patching is a reasonable one though, so I won't mind either way. A separate point -- it might be marginally more efficient to have the work of rewriteTargetListUD() done after expand_targetlist() to avoid the possible renumbering of the resjunk entries. Regards, Dean
