Re: speeding up planning with partitions

Imai-san,

Thank you for reviewing.

On 2018/11/05 17:28, Imai, Yoshikazu wrote:
> Since it takes me a lot of time to see all of the patches, I will post comments
> little by little from easy parts like typo check.

I've fixed the typos you pointed out.

> 4.
> 0004:
> -static void expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte,
> -                         Index rti);
> 
> Comments referring to expand_inherited_rtentry() is left.
> 
> backend/optimizer/plan/planner.c:1310:
>            * Because of the way expand_inherited_rtentry works, that should be
> backend/optimizer/plan/planner.c:1317:
>            * Instead the duplicate child RTE created by expand_inherited_rtentry
> backend/optimizer/util/plancat.c:118:
>     * the rewriter or when expand_inherited_rtentry() added it to the query's
> backend/optimizer/util/plancat.c:640:
>     * the rewriter or when expand_inherited_rtentry() added it to the query's
> 
> About the last two comments in the above,
> "the rewriter or when expand_inherited_rtentry() added it to the query's"
> would be
> "the rewriter or when add_inheritance_child_rel() added it to the query's".
> 
> I don't know how to correct the first two comments.

I've since modified the patch to preserve expand_inherited_rtentry name,
although with a new implementation.

> 5.
> 0004:
> -static void expand_partitioned_rtentry(PlannerInfo *root,
> -                           RangeTblEntry *parentrte,
> -                           Index parentRTindex, Relation parentrel,
> -                           PlanRowMark *top_parentrc, LOCKMODE lockmode,
> -                           List **appinfos);
> 
> Comments referring to expand_partitioned_rtentry() is also left.

I've preserved this name as well.

> backend/executor/execPartition.c:941:
>  /*
>   * get_partition_dispatch_recurse
>   *      Recursively expand partition tree rooted at rel
>   *
>   * As the partition tree is expanded in a depth-first manner, we maintain two
>   * global lists: of PartitionDispatch objects corresponding to partitioned
>   * tables in *pds and of the leaf partition OIDs in *leaf_part_oids.
>   *
>   * Note that the order of OIDs of leaf partitions in leaf_part_oids matches
>   * the order in which the planner's expand_partitioned_rtentry() processes
>   * them.  It's not necessarily the case that the offsets match up exactly,
>   * because constraint exclusion might prune away some partitions on the
>   * planner side, whereas we'll always have the complete list; but unpruned
>   * partitions will appear in the same order in the plan as they are returned
>   * here.
>   */
> 
> I think the second paragraph of the comments is no longer correct.
> expand_partitioned_rtentry() expands the partition tree in a depth-first
> manner, whereas expand_append_rel() doesn't neither in a depth-first manner
> nor a breadth-first manner as below.

Well, expand_append_rel doesn't process a whole partitioning tree on its
own, only one partitioned table at a time.  As set_append_rel_size()
traverses the root->append_rel_list, leaf partitions get added to the
resulting plan in what's effectively a depth-first order.

I've updated this comment as far this patch is concerned, although the
patch in the other thread about removal of executor overhead in
partitioning is trying to remove this function
(get_partition_dispatch_recurse) altogether anyway.

> 6.
> 0004
> +    /*
> +     * A partitioned child table with 0 children is a dummy rel, so don't
> +     * bother creating planner objects for it.
> +     */
> +    if (childrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> +    {
> +        PartitionDesc partdesc = RelationGetPartitionDesc(childrel);
> +
> +        Assert(!RELATION_IS_OTHER_TEMP(childrel));
> +        if (partdesc->nparts == 0)
> +        {
> +            heap_close(childrel, NoLock);
> +            return NULL;
> +        }
> +    }
> +
> +    /*
> +     * If childrel doesn't belong to this session, skip it, also relinquishing
> +     * the lock.
> +     */
> +    if (RELATION_IS_OTHER_TEMP(childrel))
> +    {
> +        heap_close(childrel, lockmode);
> +        return NULL;
> +    }
> 
> If we process the latter if block before the former one, Assert can be excluded
> from the code.

I've since moved the partitioning-related code to a partitioning specific
function, so this no longer applies.

Thanks,
Amit