From: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
To: Imai, Yoshikazu <imai.yoshikazu@jp.fujitsu.com>
To: jesper.pedersen@redhat.com <jesper.pedersen@redhat.com>
Cc: 'Amit Langote' <amitlangote09@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: David Rowley <david.rowley@2ndquadrant.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: speeding up planning with partitions
Date: Tue, 5 Mar 2019 16:16:52 +0900
Message-ID: <d1c18bc8-2ba2-8438-e6a7-631cd2ae8d4b@lab.ntt.co.jp> (raw)
In-Reply-To: <0F97FA9ABBDBE54F91744A9B37151A5129B9D7@g01jpexmbkw24>
References: <CAKJS1f9XAePd2+8F4=wtHKdDdckgKcSoXb-ZbU8Wbt86mhLxKA@mail.gmail.com>
<b8a139b8-2f5d-6079-67b2-99957af3fc36@lab.ntt.co.jp>
<7453.1550518972@sss.pgh.pa.us>
<0a70234e-b2e6-debc-2fb5-e8fd927caa00@lab.ntt.co.jp>
<1002.1550596071@sss.pgh.pa.us>
<30961.1550609873@sss.pgh.pa.us>
<250c61bc-479c-92f4-d19d-d768e3bcdcbc@lab.ntt.co.jp>
<13510.1550677844@sss.pgh.pa.us>
<a47e6214-bd46-ab4b-2f84-f9f726e04642@lab.ntt.co.jp>
<35b02675-83c2-a12b-961a-fec249b06f35@lab.ntt.co.jp>
<20190222165932.GG28750@telsasoft.com>
<CA+HiwqEUwfg4s=4=is-+7k-WM-2Gs9K3iFB96QNm_eafcBozqQ@mail.gmail.com>
<0F97FA9ABBDBE54F91744A9B37151A51293422@g01jpexmbkw24>
<8098beec-fc04-303d-55b9-73c3d94ba1ea@lab.ntt.co.jp>
<b5362c29-cca1-de7b-13d0-9e51f9911185@lab.ntt.co.jp>
<ce7bb7c2-8eac-0047-1e04-e771f08ad1fb@redhat.com>
<0bb0f410-bfbc-3ee5-946b-43e89e8cbd57@lab.ntt.co.jp>
<0F97FA9ABBDBE54F91744A9B37151A5129B9D7@g01jpexmbkw24>
Imai-san,
Thanks for checking.
On 2019/03/05 15:03, Imai, Yoshikazu wrote:
> 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);
Ah, it should be moved to another patch. Actually, to patch 0003, which
makes it necessary to inspect the PartitionDesc.
> 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++;
You're right. That's what makes sense in this context.
> [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.)
root->simple_rel_array_size > 0 *does* mean source inheritance expansion,
so you're not mistaken. As you can see, expand_inherited_rtentry is
called by inheritance_planner() to expand target inheritance and by
add_appendrel_other_rels() to expand source inheritance. Since the latter
is called by query_planner, simple_rel_array would have been initialized
and hence root->simple_rel_array_size > 0 is true.
Maybe it'd be a good idea to introduce is_source_inh_expansion variable
for clarity as you suggest and set it to (root->simple_rel_array_size > 0).
> 4. I didn't see much carefully, but should the introduction of contains_inherit_children be in 0003 patch...?
You're right.
Thanks again for the comments. I've made changes to my local repository.
Thanks,
Amit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: Langote_Amit_f8@lab.ntt.co.jp, imai.yoshikazu@jp.fujitsu.com, jesper.pedersen@redhat.com, amitlangote09@gmail.com, tgl@sss.pgh.pa.us, david.rowley@2ndquadrant.com, alvherre@2ndquadrant.com, pryzby@telsasoft.com
Subject: Re: speeding up planning with partitions
In-Reply-To: <d1c18bc8-2ba2-8438-e6a7-631cd2ae8d4b@lab.ntt.co.jp>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox