Re: [HACKERS] Partitioned tables and relfilenode

On 2017/03/21 1:16, Robert Haas wrote:
> On Fri, Mar 17, 2017 at 4:57 AM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>> Yes, but on the flip side, you're having to add code in a lot of
>>> places -- I think I counted 7 -- where you turn around and ignore
>>> those AppendRelInfos.
>>
>> Perhaps you were looking at the previous version with "minimal" appinfos
>> containing the child_is_partitioned field?
> 
> Yes, I think I was.  I think this version looks a lot better.

Just to clarify, I assume you reviewed the latest version which does not
create AppendRelInfos, but instead creates PartitionedChildRelInfos (as
also evident from your comments below).  Sorry about the confusion.

>      /*
> +     * Close the root partitioned rel if we opened it above, but keep the
> +     * lock.
> +     */
> +    if (rel != mtstate->resultRelInfo->ri_RelationDesc)
> +        heap_close(rel, NoLock);
> 
> We didn't take a lock above, though, so drop everything in the comment
> from "but" onward.

Oh, right.

> -    add_paths_to_append_rel(root, rel, live_childrels);
> +    add_paths_to_append_rel(root, rel, live_childrels, partitioned_rels);
> 
> I think it would make more sense to put the new logic into
> add_paths_to_append_rel, instead of passing this down as an additional
> parameter.

OK, done.

> +     * do not appear anywhere else in the plan.  Situation is exactly the
> 
> The situation is...

Fixed.

> 
> +    if (parent_rte->relkind == RELKIND_PARTITIONED_TABLE)
> +    {
> +        foreach(lc, root->pcinfo_list)
> +        {
> +            PartitionedChildRelInfo *pc = lfirst(lc);
> +
> +            if (pc->parent_relid == parentRTindex)
> +            {
> +                partitioned_rels = pc->child_rels;
> +                break;
> +            }
> +        }
> +    }
> 
> You seem to have a few copies of this logic.  I think it would be
> worth factoring it out into a separate function.

OK, done.  Put the definition in in planner.c

> +                root->glob->nonleafResultRelations =
> +                    list_concat(root->glob->nonleafResultRelations,
> +                                list_copy(splan->partitioned_rels));
> 
> Please add a brief comment.  One line is fine.

Done.

> 
> +            newrc->isParent = childrte->relkind == RELKIND_PARTITIONED_TABLE;
> 
> I'm not sure what project style is, but I personally find these kinds
> of assignments easier to read with an extra set of parantheses:
> 
>             newrc->isParent = (childrte->relkind == RELKIND_PARTITIONED_TABLE);

Ah, done.

> 
> +    if (partitioned_rels == NIL)
> +        return;
> +
> +    foreach(lc, partitioned_rels)
> 
> I think the if-test is pointless; the foreach loop is going to start
> by comparing the initial value with NIL.

Right, fixed.

> Why doesn't ExecSerializePlan() need to transfer a proper value for
> nonleafResultRelations to workers?  Seems like it should.

It doesn't transfer resultRelations either, presumably because workers do
not handle result relations yet.  Also, both resultRelations and
nonleafResultRelations are set *only* if there is a ModifyTable node.

Attached updated patches.

Thanks,
Amit

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers