Subject: Re: speeding up planning with partitions
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>
From: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Message-ID: <d1c18bc8-2ba2-8438-e6a7-631cd2ae8d4b@lab.ntt.co.jp>
Date: Tue, 5 Mar 2019 16:16:52 +0900
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.5.1
MIME-Version: 1.0
In-Reply-To: <0F97FA9ABBDBE54F91744A9B37151A5129B9D7@g01jpexmbkw24>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
To: "Imai, Yoshikazu" <imai.yoshikazu@jp.fujitsu.com>,
        "jesper.pedersen@redhat.com" <jesper.pedersen@redhat.com>
Cc: "'Amit Langote'" <amitlangote09@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
        David Rowley <david.rowley@2ndquadrant.com>,
        Alvaro Herrera <alvherre@2ndquadrant.com>,
        Justin Pryzby <pryzby@telsasoft.com>,
        Pg Hackers <pgsql-hackers@postgresql.org>
Precedence: bulk

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