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

Imai-san,

Thanks for the review.

On 2019/03/04 18:14, Imai, Yoshikazu wrote:
> I've taken at glance the codes and there are some minor comments about the patch.
> 
> * You changed a usage/arguments of get_relation_info, but why you did it? I made those codes back to the original
codeand checked it passes make check. So ISTM there are no logical problems with not changing it. Or if you change it,
howabout also change a usage/arguments of get_relation_info_hook in the same way?
 
> 
> -get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent,
> -                  RelOptInfo *rel)
> +get_relation_info(PlannerInfo *root, RangeTblEntry *rte, RelOptInfo *rel)
>  {
> +    bool        inhparent = rte->inh;
> -    relation = table_open(relationObjectId, NoLock);
> +    relation = heap_open(rte->relid, NoLock);
>   ...
>      if (get_relation_info_hook)
> -        (*get_relation_info_hook) (root, relationObjectId, inhparent, rel);
> +        (*get_relation_info_hook) (root, rte->relid, rte->inh, rel);
> 
> 
> @@ -217,15 +272,13 @@ build_simple_rel(PlannerInfo *root, int relid, RelOptInfo *parent)
>      /* Check type of rtable entry */
>      switch (rte->rtekind)
>      {
>          case RTE_RELATION:
>              /* Table --- retrieve statistics from the system catalogs */
> -            get_relation_info(root, rte->relid, rte->inh, rel);
> +            get_relation_info(root, rte, rel);
> 
> 
> * You moved the codes of initializing of append rel's partitioned_child_rels in set_append_rel_size() to
build_simple_rel(),but is it better to do? I also checked the original code passes make check by doing like above.
 
> 
> @@ -954,32 +948,6 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
>      Assert(IS_SIMPLE_REL(rel));
>  
>      /*
> -     * Initialize partitioned_child_rels to contain this RT index.
> -     *
> -     * Note that during the set_append_rel_pathlist() phase, we will bubble up
> -     * the indexes of partitioned relations that appear down in the tree, so
> -     * that when we've created Paths for all the children, the root
> -     * partitioned table's list will contain all such indexes.
> -     */
> -    if (rte->relkind == RELKIND_PARTITIONED_TABLE)
> -        rel->partitioned_child_rels = list_make1_int(rti);
> 
> @@ -274,55 +327,287 @@ build_simple_rel(PlannerInfo *root, int relid, RelOptInfo *parent)
>                                          list_length(rte->securityQuals));
>  
>      /*
> -     * If this rel is an appendrel parent, recurse to build "other rel"
> -     * RelOptInfos for its children.  They are "other rels" because they are
> -     * not in the main join tree, but we will need RelOptInfos to plan access
> -     * to them.
> +     * Add the parent to partitioned_child_rels.
> +     *
> +     * Note that during the set_append_rel_pathlist() phase, values of the
> +     * indexes of partitioned relations that appear down in the tree will
> +     * be bubbled up into root parent's list so that when we've created
> +     * Paths for all the children, the root table's list will contain all
> +     * such indexes.
>       */
> -    if (rte->inh)
> +    if (rel->part_scheme)
> +        rel->partitioned_child_rels = list_make1_int(relid);

Both of these changes are not present in the latest patches I posted,
where I also got rid of a lot of unnecessary diffs.

Thanks,
Amit