RE: speeding up planning with partitions

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>, "jesper(dot)pedersen(at)redhat(dot)com" <jesper(dot)pedersen(at)redhat(dot)com>
Cc: 'Amit Langote' <amitlangote09(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Justin Pryzby <pryzby(at)telsasoft(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: RE: speeding up planning with partitions
Date: 2019-03-05 06:03:17
Message-ID: 0F97FA9ABBDBE54F91744A9B37151A5129B9D7@g01jpexmbkw24
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Amit-san,

On Tue, Mar 5, 2019 at 0:51 AM, Amit Langote wrote:
> Hi Jesper,
>
> Thanks for the review. I've made all of the changes per your comments
> in my local repository. I'll post the updated patches after diagnosing
> what I'm suspecting a memory over-allocation bug in one of the patches.
> If you configure build with --enable-cassert, you'll see that throughput
> decreases as number of partitions run into many thousands, but it doesn't
> when asserts are turned off.
>
> On 2019/03/05 1:20, Jesper Pedersen wrote:
> > I'll run some tests using a hash partitioned setup.
>
> Thanks.

I've also done code review of 0001 and 0002 patch so far.

[0001]
1. Do we need to open/close a relation in add_appendrel_other_rels()?

+ if (rel->part_scheme)
{
- ListCell *l;
...
- Assert(cnt_parts == nparts);
+ rel->part_rels = (RelOptInfo **)
+ palloc0(sizeof(RelOptInfo *) * rel->nparts);
+ relation = table_open(rte->relid, NoLock);
}

+ if (relation)
+ table_close(relation, NoLock);

2. We can sophisticate the usage of Assert in add_appendrel_other_rels().

+ if (rel->part_scheme)
{
...
+ rel->part_rels = (RelOptInfo **)
+ palloc0(sizeof(RelOptInfo *) * rel->nparts);
+ relation = table_open(rte->relid, NoLock);
}
...
+ i = 0;
+ foreach(l, root->append_rel_list)
+ {
...
+ if (rel->part_scheme != NULL)
+ {
+ Assert(rel->nparts > 0 && i < rel->nparts);
+ rel->part_rels[i] = childrel;
+ }
+
+ i++;

as below;

+ if (rel->part_scheme)
{
...
Assert(rel->nparts > 0);
+ rel->part_rels = (RelOptInfo **)
+ palloc0(sizeof(RelOptInfo *) * rel->nparts);
+ relation = table_open(rte->relid, NoLock);
}
...
+ i = 0;
+ foreach(l, root->append_rel_list)
+ {
...
+ if (rel->part_scheme != NULL)
+ {
+ Assert(i < rel->nparts);
+ rel->part_rels[i] = childrel;
+ }
+
+ i++;

[0002]
3. If using variable like is_source_inh_expansion, the code would be easy to read. (I might mistakenly understand root->simple_rel_array_size > 0 means source inheritance expansion though.)

In expand_inherited_rtentry() and expand_partitioned_rtentry():

+ * Expand planner arrays for adding the child relations. Can't do
+ * this if we're not being called from query_planner.
+ */
+ if (root->simple_rel_array_size > 0)
+ {
+ /* Needed when creating child RelOptInfos. */
+ rel = find_base_rel(root, rti);
+ expand_planner_arrays(root, list_length(inhOIDs));
+ }

+ /* Create the otherrel RelOptInfo too. */
+ if (rel)
+ (void) build_simple_rel(root, childRTindex, rel);

would be:

+ * Expand planner arrays for adding the child relations. Can't do
+ * this if we're not being called from query_planner.
+ */
+ if (is_source_inh_expansion)
+ {
+ /* Needed when creating child RelOptInfos. */
+ rel = find_base_rel(root, rti);
+ expand_planner_arrays(root, list_length(inhOIDs));
+ }

+ /* Create the otherrel RelOptInfo too. */
+ if (is_source_inh_expansion)
+ (void) build_simple_rel(root, childRTindex, rel);

4. I didn't see much carefully, but should the introduction of contains_inherit_children be in 0003 patch...?

I'll continue to do code review of rest patches.

--
Yoshikazu Imai

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2019-03-05 06:08:04 Re: Inheriting table AMs for partitioned tables
Previous Message David Rowley 2019-03-05 05:55:22 Re: Delay locking partitions during query execution