Re: speeding up planning with partitions

Imai-san,

Thanks for the review.

On 2019/03/06 11:09, Imai, Yoshikazu wrote:
> Here is the code review for previous v26 patches.
> 
> [0002]
> In expand_inherited_rtentry():
> 
> expand_inherited_rtentry()
> {
>     ...
> +   RelOptInfo *rel = NULL;
> 
> can be declared at more later:
> 
> if (oldrelation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> ...
> else
> {   
>     List       *appinfos = NIL;
>     RangeTblEntry *childrte;
>     Index       childRTindex;
> +    RelOptInfo *rel = NULL;
> 
> 

This is fixed in v27.

> [0003]
> In inheritance_planner:
> 
> +    rtable_with_target = list_copy(root->parse->rtable);
> 
> can be:
> 
> +    rtable_with_target = list_copy(parse->rtable);

Sure.

> [0004 or 0005]
> There are redundant process in add_appendrel_other_rels (or expand_xxx_rtentry()?).
> I modified add_appendrel_other_rels like below and it actually worked.
> 
> 
> add_appendrel_other_rels(PlannerInfo *root, RangeTblEntry *rte, Index rti) 
> {
>     ListCell       *l;
>     RelOptInfo     *rel;
> 
>     /*   
>      * Add inheritance children to the query if not already done. For child
>      * tables that are themselves partitioned, their children will be added
>      * recursively.
>      */
>     if (rte->rtekind == RTE_RELATION && !root->contains_inherit_children)
>     {    
>         expand_inherited_rtentry(root, rte, rti);
>         return;
>     }    
> 
>     rel = find_base_rel(root, rti);
> 
>     foreach(l, root->append_rel_list)
>     {    
>         AppendRelInfo  *appinfo = lfirst(l);
>         Index           childRTindex = appinfo->child_relid;
>         RangeTblEntry  *childrte;
>         RelOptInfo     *childrel;
> 
>         if (appinfo->parent_relid != rti) 
>                 continue;
> 
>         Assert(childRTindex < root->simple_rel_array_size);
>         childrte = root->simple_rte_array[childRTindex];
>         Assert(childrte != NULL);
>         build_simple_rel(root, childRTindex, rel);
> 
>         /* Child may itself be an inherited relation. */
>         if (childrte->inh)
>             add_appendrel_other_rels(root, childrte, childRTindex);
>     }    
> }

If you don't intialize part_rels here, then it will break any code in the
planner that expects a partitioned rel with non-zero number of partitions
to have part_rels set to non-NULL.  At the moment, that includes the code
that implements partitioning-specific optimizations such partitionwise
aggregate and join, run-time pruning etc.  No existing regression tests
cover the cases where source inherited roles participates in those
optimizations, so you didn't find anything that broke.  For an example,
consider the following update query:

update p set a = p1.a + 1 from p p1 where p1.a = (select 1);

Planner will create a run-time pruning aware Append node for p (aliased
p1), for which it will need to use the part_rels array.  Note that p in
this case is a source relation which the above code initializes.

Maybe we should add such regression tests.

> I didn't investigate that problem, but there is another memory increase
issue, which is because of 0003 patch I think. I'll try to solve the
latter issue.

Interested in details as it seems to be a separate problem.

Thanks,
Amit