From: | "Imai, Yoshikazu" <imai(dot)yoshikazu(at)jp(dot)fujitsu(dot)com> |
---|---|
To: | 'Amit Langote' <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>, Amit Langote <amitlangote09(at)gmail(dot)com> |
Cc: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | RE: speeding up planning with partitions |
Date: | 2019-02-08 04:44:16 |
Message-ID: | 0F97FA9ABBDBE54F91744A9B37151A5127865E@g01jpexmbkw24 |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Amit-san,
On Thu, Feb 7, 2019 at 10:22 AM, Amit Langote wrote:
> Rebased over bdd9a99aac.
I did code review of 0001 and I have some suggestions. Could you check them?
1.
0001: line 418
+ * add_inherit_target_child_root() would've added only those that are
add_inherit_target_child_root() doesn't exist now, so an above comment needs to be modified.
2.
0001: line 508-510
In set_inherit_target_rel_pathlists():
+ /* Nothing to do if all the children were excluded. */
+ if (IS_DUMMY_REL(rel))
+ return;
These codes aren't needed or can be replaced by Assert because set_inherit_target_rel_pathlists is only called from set_rel_pathlist which excutes IS_DUMMY_REL(rel) before calling set_inherit_target_rel_pathlists, as follows.
set_rel_pathlist(...)
{
...
if (IS_DUMMY_REL(rel))
{
/* We already proved the relation empty, so nothing more to do */
}
else if (rte->inh)
{
/*
* If it's a target relation, set the pathlists of children instead.
* Otherwise, we'll append the outputs of children, so process it as
* an "append relation".
*/
if (root->inherited_update && root->parse->resultRelation == rti)
{
inherited_update = true;
set_inherit_target_rel_pathlists(root, rel, rti, rte);
3.
0001: line 1919-1920
- case CONSTRAINT_EXCLUSION_ON:
- break; /* always try to exclude */
CONSTRAINT_EXCLUSION_ON is no longer used, so should we remove it also from guc parameters?
4.
0001:
Can we remove enum InheritanceKind which is no longer used?
I also see the patch from a perspective of separating codes from 0001 which are not essential of Overhaul inheritance update/delete planning. Although almost all of codes are related each other, but I found below two things can be moved to another patch.
---
0001: line 550-608
This section seems to be just refinement of set_append_rel_size().
So can we separate this from 0001 to another patch?
---
0001: line 812-841, 940-947, 1525-1536, 1938-1947
These codes are related to removement of InheritanceKind from relation_excluded_by_constraints(), so I think it is something like cleaning of unneeded codes. Can we separate this to patch as some-code-clearnups-of-0001.patch? Of course, we can do that only if removing of these codes from 0001 would not bother success of "make check" of 0001.
I also think that what I pointed out at above 3. and 4. can also be included in some-code-clearnups-of-0001.patch.
What do you think?
--
Yoshikazu Imai
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2019-02-08 04:56:18 | Re: Synchronize with imath upstream |
Previous Message | Noah Misch | 2019-02-08 04:38:44 | Re: Synchronize with imath upstream |