On 2016/11/02 2:53, Robert Haas wrote: > On Fri, Oct 28, 2016 at 3:53 AM, Amit Langote > <langote_amit...@lab.ntt.co.jp> wrote: >> [ new patches ] > > Reviewing 0005: > > Your proposed commit messages says this: > >> If relation is the target table (UPDATE and DELETE), flattening is >> done regardless (scared to modify inheritance_planner() yet).
I should have explained my thinking behind why I ended up not handling the result relation case: While set_append_rel_size() can be safely invoked recursively on the roots of partition sub-trees, I was not quite sure if inheritance_planner() is amenable to such recursive invocation. While the former is relatively straightforward baserel processing, the latter is not-so-straightforward transformation of the whole query for every target child relation of the target parent. > In the immortal words of Frank Herbert: “I must not fear. Fear is the > mind-killer. Fear is the little-death that brings total obliteration. > I will face my fear. I will permit it to pass over me and through me. > And when it has gone past I will turn the inner eye to see its path. > Where the fear has gone there will be nothing. Only I will remain.” > > In other words, I'm not going to accept fear as a justification for > randomly-excluding the target-table case from this code. If there's > an actual reason not to do this in that case or some other case, then > let's document that reason. But weird warts in the code that are > justified only by nameless anxiety are not good. Perhaps the above comment expands a little on the actual reason. Though I guess it still amounts to giving up on doing a full analysis of whether recursive processing within inheritance_planner() is feasible. I think we could just skip this patch as long as a full investigation into inheritance_planner() issues is not done. It's possible that there might be other places in the planner code which are not amenable to the proposed multi-level AppendRelInfos. Ashutosh had reported one [1], wherein lateral joins wouldn't work with multi-level partitioned table owing to the multi-level AppendRelInfos (the patch contains a hunk to address that). > Of course, the prior question is whether we should EVER be doing this. > I realize that something like this MAY be needed for partition-wise > join, but the mission of this patch is not to implement partition-wise > join. Does anything in this patch series really require this? If so, > what? If not, how about we leave it out and refactor it when that > change is really needed for something? Nothing *requires* it as such. One benefit I see is that exclusion of the root of some partition sub-tree means the whole sub-tree is excluded in one go. Currently, because of the flattening, every relation in that sub-tree would be excluded separately, needlessly repeating the expensive constraint exclusion proof. But I guess that's just an optimization. Thanks, Amit [1] https://www.postgresql.org/message-id/CAFjFpReZF34MDbY95xoATi0xVj2mAry4-LHBWVBayOc8gj%3Diqg%40mail.gmail.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers