Re: speeding up planning with partitions

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: 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: 2018-11-10 11:59:50
Message-ID: CAKJS1f8g9_BzE678BLBm-eoMMEYUUXhDABSpqtAHRUUTrm_vFA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On 9 November 2018 at 21:55, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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

Attachment Content-Type Size
verify_em_child.diff application/octet-stream 11.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Daniel Westermann 2018-11-10 15:21:06 Re: zheap: a new storage format for PostgreSQL
Previous Message denty 2018-11-10 10:43:43 Re: Delta Materialized View Refreshes?