Re: speeding up planning with partitions

On 9 November 2018 at 21:55, Amit Langote <Langote_Amit_f8@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