Re: speeding up planning with partitions

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