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
