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 code and checked it passes make check.
> So ISTM there are no logical problems with not changing it. Or if you change
> it, how about 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
