On Thu, 10 Jan 2019 at 21:41, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> In the v13 that I will try to post tomorrow, I will have hopefully
> addressed David's and Imai-san's review comments. Thank you both!
I'd been looking at v11's 0002 and started on 0003 too. I'll include
my notes so far if you're about to send a v13.
v11 0002
18. There's a missing case in the following code. I understand that
makeNode() will 0 the member here, but that does not stop the various
other initialisations that set the default value for the field. Below
there's a missing case where parent != NULL && parent->rtekind !=
RTE_RELATION. You might be better just always zeroing the field below
"rel->partitioned_child_rels = NIL;"
+
+ /*
+ * For inheritance child relations, we also set inh_root_parent.
+ * Note that 'parent' might itself be a child (a sub-partitioned
+ * partition), in which case we simply use its value of
+ * inh_root_parent.
+ */
+ if (parent->rtekind == RTE_RELATION)
+ rel->inh_root_parent = parent->inh_root_parent > 0 ?
+ parent->inh_root_parent :
+ parent->relid;
}
else
+ {
rel->top_parent_relids = NULL;
+ rel->inh_root_parent = 0;
+ }
19. Seems strange to have a patch that adds a new field that is
unused. I also don't quite understand yet why top_parent_relids can't
be used instead. I think I recall being confused about that before, so
maybe it's worth writing a comment to mention why it cannot be used.
v11 0003
20. This code looks wrong:
+ /*
+ * expand_inherited_tables may have proved that the relation is empty, so
+ * check if it's so.
+ */
+ else if (rte->inh && !IS_DUMMY_REL(rel))
Likely you'll want:
else if rte->inh)
{
if (IS_DUMMY_REL(rel))
return;
// other stuff
}
otherwise, you'll end up in the else clause when you shouldn't be.
21. is -> was
+ * The child rel's RelOptInfo is created during
+ * expand_inherited_tables().
*/
childrel = find_base_rel(root, childRTindex);
since you're talking about something that already happened.
I'll continue looking at v12.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
