Re: speeding up planning with partitions - Mailing list pgsql-hackers

Imai-san,

Thank you for reviewing.

On 2018/10/04 17:11, Imai, Yoshikazu wrote:
> Hi, Amit!
> 
> On Thu, Sept 13, 2018 at 10:29 PM, Amit Langote wrote:
>> Attached is what I have at the moment.
> 
> I also do the code review of the patch.
> I could only see a v3-0001.patch so far, so below are all about v3-0001.patch.
> 
> I am new to inheritance/partitioning codes and code review, so my review below might have some mistakes. If there are
mistakes,please point out that kindly :)
 
> 
> 
> v3-0001:
> 1. Is there any reason inheritance_make_rel_from_joinlist returns "parent" that is passed to it? I think we can refer
parentin the caller even if inheritance_make_rel_from_joinlist returns nothing.
 
>
> +static RelOptInfo *
> +inheritance_make_rel_from_joinlist(PlannerInfo *root,
> +                                   RelOptInfo *parent,
> +                                   List *joinlist)
> +{
>     ...
> +    return parent;
> +}

There used to be a reason to do that in previous versions, but seems it
doesn't hold anymore.  I've changed it to not return any value.

> 2. Is there the possible case that IS_DUMMY_REL(child_joinrel) is true in inheritance_adjust_scanjoin_target()?
> 
> +inheritance_adjust_scanjoin_target(PlannerInfo *root,
>     ...
> +{
>     ...
> +    foreach(lc, root->inh_target_child_rels)
> +    {
>         ...
> +        /*
> +         * If the child was pruned, its corresponding joinrel will be empty,
> +         * which we can ignore safely.
> +         */
> +        if (IS_DUMMY_REL(child_joinrel))
> +            continue;
> 
> I think inheritance_make_rel_from_joinlist() doesn't make dummy joinrel for the child that was pruned.
> 
> +inheritance_make_rel_from_joinlist(PlannerInfo *root,
> ...
> +{
>     ...
> +    foreach(lc, root->append_rel_list)
> +    {
> +        RelOptInfo *childrel;
>         ...
> +        /* Ignore excluded/pruned children. */
> +        if (IS_DUMMY_REL(childrel))
> +            continue;
>         ...
> +        /*
> +         * Save child joinrel to be processed later in
> +         * inheritance_adjust_scanjoin_target, which adjusts its paths to
> +         * be able to emit expressions in query's top-level target list.
> +         */
> +        root->inh_target_child_rels = lappend(root->inh_target_child_rels,
> +                                              childrel);
> +    }
> +}

You're right.  Checking that in inheritance_adjust_scanjoin_target was
redundant.

> 3.
> Is the "root->parse->commandType != CMD_INSERT" required in:
> 
> @@ -2018,13 +1514,45 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
> ...
> +        /*
> +         * For UPDATE/DELETE on an inheritance parent, we must generate and
> +         * apply scanjoin target based on targetlist computed using each
> +         * of the child relations.
> +         */
> +        if (parse->commandType != CMD_INSERT &&
> +            current_rel->reloptkind == RELOPT_BASEREL &&
> +            current_rel->relid == root->parse->resultRelation &&
> +            root->simple_rte_array[current_rel->relid]->inh)
> ...
> 
> @@ -2137,92 +1665,123 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
>      final_rel->fdwroutine = current_rel->fdwroutine;
>  
> ...
> -    foreach(lc, current_rel->pathlist)
> +    if (current_rel->reloptkind == RELOPT_BASEREL &&
> +        current_rel->relid == root->parse->resultRelation &&
> +        !IS_DUMMY_REL(current_rel) &&
> +        root->simple_rte_array[current_rel->relid]->inh &&
> +        parse->commandType != CMD_INSERT)
> 
> 
> I think if a condition would be "current_rel->relid == root->parse->resultRelation && parse->commandType !=
CMD_INSERT"at the above if clause, elog() is called in the query_planner and planner don't reach above if clause.
 
> Of course there is the case that query_planner returns the dummy joinrel and elog is not called, but in that case,
current_rel->relidmay be 0(current_rel is dummy joinrel) and root->parse->resultRelation may be not 0(a query is
INSERT).

Yeah, I realized that we can actually Assert(parse->commandType !=
CMD_INSERT) if the inh flag of the target/resultRelation RTE is true.  So,
checking it explicitly is redundant.

> 4. Can't we use define value(IS_PARTITIONED or something like IS_INHERITANCE?) to identify inheritance and
partitionedtable in below code? It was little confusing to me that which code is related to inheritance/partitioned
whenlooking codes.
 
> 
> In make_one_rel():
> +    if (root->parse->resultRelation &&
> +        root->simple_rte_array[root->parse->resultRelation]->inh)
> +    {
> ...
> +        rel = inheritance_make_rel_from_joinlist(root, targetrel, joinlist);
> 
> In inheritance_make_rel_from_joinlist():
> +        if (childrel->part_scheme != NULL)
> +            childrel =
> +                inheritance_make_rel_from_joinlist(root, childrel,
> +                                                   translated_joinlist);

As it might have been clear, the value of inh flag is checked to detect
whether the operation uses inheritance, because it requires special
handling -- the operation must be applied to every child relation that
satisfies the conditions of the query.

part_scheme is checked to detect partitioning which has some special
behavior with respect to how the AppendRelInfos are set up.  For
partitioning, AppendRelInfos map partitions to their immediate parent, not
the top-most root parent, so the current code uses recursion to apply
certain transformations.  That's not required for non-partitioned case,
because all children are mapped to the root inheritance parent.

I'm trying to unify the two so that the partitioning case doesn't need any
special handling.

> I can't review inheritance_adjust_scanjoin_target() deeply, because it is difficult to me to understand fully codes
aboutjoin processing.
 

Thanks for your comments, they are valuable.

Regards,
Amit