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