On 9 November 2018 at 21:55, Amit Langote <langote_amit...@lab.ntt.co.jp> wrote: > v5-0001-Store-inheritance-root-parent-index-in-otherrel-s.patch > > Adds a inh_root_parent field that's set in inheritance child otherrel > RelOptInfos to store the RT index of their root parent > > v5-0002-Overhaul-inheritance-update-delete-planning.patch > > Patch that adjusts planner so that inheritance_planner can use partition > pruning.
I've started looking at these two, but only so far made it through 0001 and 0002. Here's what I noted down during the review. 0001: 1. Why do we need the new field that this patch adds? I see in 0002 it's used like: + if (childrel->inh_root_parent > 0 && + childrel->inh_root_parent == root->parse->resultRelation) Would something like... int relid; if (childrel->part_schema == NULL && bms_get_singleton_member(childrel->top_parent_relids, &relid) && relid == root->parse->resultRelation) ...not do the trick? 0002: 2. What's wrong with childrel->relids? + child_relids = bms_make_singleton(appinfo->child_relid); 3. Why not use childrel->top_parent_relids? + top_parent_relids = bms_make_singleton(childrel->inh_root_parent); 4. The following comment in inheritance_make_rel_from_joinlist() implies that the function will not be called for SELECT, but the comment above the function does not mention that. /* * For UPDATE/DELETE queries, the top parent can only ever be a table. * As a contrast, it could be a UNION ALL subquery in the case of SELECT. */ Assert(planner_rt_fetch(top_parent_relid, root)->rtekind == RTE_RELATION); 5. Should the following comment talk about "Sub-partitioned tables" rather than "sub-partitions"? + * Sub-partitions have to be processed recursively, because + * AppendRelInfos link sub-partitions to their immediate parents, not + * the root partitioned table. 6. Can't you just pass childrel->relids and childrel->top_parent_relids instead of making new ones? + child_relids = bms_make_singleton(appinfo->child_relid); + Assert(childrel->inh_root_parent > 0); + top_parent_relids = bms_make_singleton(childrel->inh_root_parent); + translated_joinlist = (List *) + adjust_appendrel_attrs_multilevel(root, + (Node *) joinlist, + child_relids, + top_parent_relids); 7. I'm just wondering what your idea is here? + /* Reset join planning specific data structures. */ + root->join_rel_list = NIL; + root->join_rel_hash = NULL; Is there a reason to nullify these? You're not releasing any memory and the new structures that will be built won't overlap with the ones built last round. I don't mean to imply that the code is wrong, it just does not appear to be particularly right. 8. In regards to: + * NB: Do we need to change the child EC members to be marked + * as non-child somehow? + */ + childrel->reloptkind = RELOPT_BASEREL; I know we talked a bit about this before, but this time I put together a crude patch that runs some code each time we skip an em_is_child == true EquivalenceMember. The code checks if any of the em_relids are RELOPT_BASEREL. What I'm looking for here are places where we erroneously skip the member when we shouldn't. Running the regression tests with this patch in place shows a number of problems. Likely I should only trigger the warning when bms_membership(em->em_relids) == BMS_SINGLETON, but it never-the-less appears to highlight various possible issues. Applying the same on master only appears to show the cases where em->em_relids isn't a singleton set. I've attached the patch to let you see what I mean. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
verify_em_child.diff
Description: Binary data
