Amit Langote <langote_amit...@lab.ntt.co.jp> writes:
> On 2019/02/19 4:42, Tom Lane wrote:
>> I don't much care for re-calling build_base_rel_tlists to add extra
>> Vars to the appropriate relations; 99% of the work it does will be
>> wasted, and with enough child rels you could run into an O(N^2)
>> problem. Maybe you could call add_vars_to_targetlist directly,
>> since you know exactly what Vars you're adding?
> Assuming you're talking about the build_base_rel_tlists() call in
> create_inherited_target_child_root(), it's necessary because *all* tlist
> Vars are new, because the targetlist has just been translated at that
> point. But maybe I missed your point?
Hmm, I'll take a closer look --- I thought it was just there to add the
new ctid-or-equivalent columns. Didn't our earlier translation of the
whole subroot structure reach the reltargetlists?
> By the way, later patch in the series will cause partition pruning to
> occur before these child PlannerInfos are generated, so
> create_inherited_target_child_root will be called only as many times as
> there are un-pruned child target relations.
Well, that helps, but only for "point update" queries. You still need
to be wary of not causing O(N^2) behavior when there are lots of unpruned
partitions.
>> What is the point of moving the calculation of all_baserels? The earlier
>> you construct that, the more likelihood that code will have to be written
>> to modify it (like, say, what you had to put into
>> create_inherited_target_child_root), and I do not see anything in this
>> patch series that needs it to be available earlier.
> all_baserels needs to be built in original PlannerInfo before child
> PlannerInfos are constructed, so that they can simply copy it and have the
> parent target baserel RT index in it replaced by child target baserel RT
> index. set_inherited_target_rel_sizes/pathlists use child PlannerInfos,
> so all_baserels must be set in them just like it is in the original
> PlannerInfo.
No, I think you're not getting my point: if you build all_baserels in
the same place where it already is being built, you don't need to do
any of that because it's already correct. It looks to me like this
code motion is left over from some earlier iteration of the patch where
it probably was necessary, but now it's just making your life harder.
>> Couldn't inh_target_child_rels list be removed in favor of looking
>> at the relid fields of the inh_target_child_path_rels entries?
>> Having to keep those two lists in sync seems messy.
> Don't like having two lists either, but inh_target_child_path_rels entries
> can be RELOPT_JOINREL, so relid can be 0.
So? For a join, there's no particularly relevant integer to put into
inh_target_child_rels either. (I might like these two lists better
if they had better-chosen names. Merely making them long enough to
induce carpal tunnel syndrome isn't helping distinguish them.)
> Attached updated patches. 0001 that I'd previously posted is no longer
> included, as I said in the other email.
OK, I'll make another pass over 0001 today.
regards, tom lane
