Imai-san,
Thanks for checking.
On 2019/03/05 15:03, Imai, Yoshikazu wrote:
> I've also done code review of 0001 and 0002 patch so far.
>
> [0001]
> 1. Do we need to open/close a relation in add_appendrel_other_rels()?
>
> + if (rel->part_scheme)
> {
> - ListCell *l;
> ...
> - Assert(cnt_parts == nparts);
> + rel->part_rels = (RelOptInfo **)
> + palloc0(sizeof(RelOptInfo *) * rel->nparts);
> + relation = table_open(rte->relid, NoLock);
> }
>
> + if (relation)
> + table_close(relation, NoLock);
Ah, it should be moved to another patch. Actually, to patch 0003, which
makes it necessary to inspect the PartitionDesc.
> 2. We can sophisticate the usage of Assert in add_appendrel_other_rels().
>
> + if (rel->part_scheme)
> {
> ...
> + rel->part_rels = (RelOptInfo **)
> + palloc0(sizeof(RelOptInfo *) * rel->nparts);
> + relation = table_open(rte->relid, NoLock);
> }
> ...
> + i = 0;
> + foreach(l, root->append_rel_list)
> + {
> ...
> + if (rel->part_scheme != NULL)
> + {
> + Assert(rel->nparts > 0 && i < rel->nparts);
> + rel->part_rels[i] = childrel;
> + }
> +
> + i++;
>
> as below;
>
> + if (rel->part_scheme)
> {
> ...
> Assert(rel->nparts > 0);
> + rel->part_rels = (RelOptInfo **)
> + palloc0(sizeof(RelOptInfo *) * rel->nparts);
> + relation = table_open(rte->relid, NoLock);
> }
> ...
> + i = 0;
> + foreach(l, root->append_rel_list)
> + {
> ...
> + if (rel->part_scheme != NULL)
> + {
> + Assert(i < rel->nparts);
> + rel->part_rels[i] = childrel;
> + }
> +
> + i++;
You're right. That's what makes sense in this context.
> [0002]
> 3. If using variable like is_source_inh_expansion, the code would be easy to
> read. (I might mistakenly understand root->simple_rel_array_size > 0 means
> source inheritance expansion though.)
root->simple_rel_array_size > 0 *does* mean source inheritance expansion,
so you're not mistaken. As you can see, expand_inherited_rtentry is
called by inheritance_planner() to expand target inheritance and by
add_appendrel_other_rels() to expand source inheritance. Since the latter
is called by query_planner, simple_rel_array would have been initialized
and hence root->simple_rel_array_size > 0 is true.
Maybe it'd be a good idea to introduce is_source_inh_expansion variable
for clarity as you suggest and set it to (root->simple_rel_array_size > 0).
> 4. I didn't see much carefully, but should the introduction of
> contains_inherit_children be in 0003 patch...?
You're right.
Thanks again for the comments. I've made changes to my local repository.
Thanks,
Amit
