From: | David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> |
---|---|
To: | Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> |
Cc: | Amit Langote <amitlangote09(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, "Imai, Yoshikazu" <imai(dot)yoshikazu(at)jp(dot)fujitsu(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: speeding up planning with partitions |
Date: | 2019-01-10 09:12:46 |
Message-ID: | CAKJS1f8GG6q44t=dEsSxsy51i5dA0LZaP=VkwXg+VUNoqVVBww@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, 10 Jan 2019 at 21:41, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Dean Rasheed | 2019-01-10 09:18:09 | Re: BUG #15446: Crash on ALTER TABLE |
Previous Message | Fabien COELHO | 2019-01-10 09:12:26 | Re: [HACKERS] pgbench - allow to store select results into variables |