On Tue, Sep 12, 2017 at 7:31 AM, Amit Langote <langote_amit...@lab.ntt.co.jp> wrote: > On 2017/09/11 19:45, Ashutosh Bapat wrote: >> On Mon, Sep 11, 2017 at 12:16 PM, Amit Langote wrote: >>> IMHO, we should make it the responsibility of the future patch to set a >>> child PlanRowMark's prti to the direct parent's RT index, when we actually >>> know that it's needed for something. We clearly know today why we need to >>> pass the other objects like child RT entry, RT index, and Relation, so we >>> should limit this patch to pass only those objects to the recursive call. >>> That makes this patch a relatively easy to understand change. >> >> I think you are mixing two issues here 1. setting parent RTI in child >> PlanRowMark and 2. passing immediate parent's PlanRowMark to >> expand_single_inheritance_child(). >> >> I have discussed 1 in my reply to Robert. >> >> About 2 you haven't given any particular comments to my reply. To me >> it looks like it's this patch that introduces the notion of >> multi-level expansion, so it's natural for this patch to pass >> PlanRowMark in cascaded fashion similar to other structures. > > You patch does 2 to be able to do 1, doesn't it? That is, to be able to > set the child PlanRowMark's prti to the direct parent's RT index, you pass > the immediate parent's PlanRowMark to the recursive call of > expand_single_inheritance_child().
No. child PlanRowMark's prti is set to parentRTIndex, which is a separate argument and is used to also set parent_relid in AppendRelInfo. > >> Actually, the original problem that caused this discussion started >> with an assertion failure in get_partitioned_child_rels() as >> Assert(list_length(result) >= 1); >> >> This assertion fails if result is NIL when an intermediate partitioned >> table is passed. May be we should assert (result == NIL || >> list_length(result) == 1) and allow that function to be called even >> for intermediate partitioned partitions for which the function will >> return NIL. That will leave the code in add_paths_to_append_rel() >> simple. Thoughts? > > Yeah, I guess that could work. We'll just have to write comments to > describe why the Assert is written that way. > >>> By the way, when we call expand_single_inheritance_child() in the >>> non-partitioned inheritance case, we should pass NULL for childrte_p, >>> childRTindex_p, childrc_p, instead of declaring variables that won't be >>> used. Hence, expand_single_inheritance_child() should make those >>> arguments optional. >> >> That introduces an extra "if" condition, which is costlier than an >> assignment. We have used both the styles in the code. Previously, I >> have got comments otherwise. So, I am not sure. > > OK. expand_single_inheritance_child's header comment does not mention the > new result fields. Maybe add a comment describing what their role is and > that they're not optional arguments. > >> I will update the patches once we have some resolution about 1. prti >> in PlanRowMarks and 2. detection of root partitioned table in >> add_paths_to_append_rel(). > > OK. > > About 2, I somewhat agree with your proposed solution above, which might > be simpler to explain in comments than the code I proposed. After testing a few queries I am getting a feeling that ExecLockNonLeafAppendTables isn't really locking anything. I will write more about that in my reply to Robert's mail. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
