Re: speeding up planning with partitions - Mailing list pgsql-hackers

Thanks for the review.

On 2019/02/19 4:42, Tom Lane wrote:
> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
>> [ v22 patch set ]
> 
> I did some review of the 0002 patch.  I like the general idea,
> but there are a lot of details I don't much like.
> 
> Probably the biggest complaint is that I don't like what you did to the
> API of grouping_planner(): it's ugly and unprincipled.  Considering that
> only a tiny fraction of what grouping_planner does is actually relevant to
> inherited UPDATES/DELETEs, maybe we should factor that part out --- or,
> heavens, just duplicate some code --- so that inheritance_planner doesn't
> need to call grouping_planner at all.  (I see that you have another patch
> in the queue that proposes to fix this through a rather massive
> refactoring of grouping_planner, but I do not think I like that approach.
> grouping_planner does perhaps need refactoring, but I don't want to drive
> such work off an arbitrary-and-dubious requirement that it shouldn't call
> query_planner.)

OK, modified the patch to leave grouping_planner unchanged (except got rid
of inheritance_update argument).  Now inheritance_planner directly
modifies the paths produced by query_planner (per child target relation
that is), so that they have a PathTarget suitable for producing query's
top-level targetlist.  I refactored portion of
apply_scanjoin_target_to_paths() that does that work and
inheritance_planner calls it directly.

> Another point is that I don't like this division of labor between
> equivclass.c and appendinfo.c.  I don't like exposing add_eq_member
> globally --- that's just an invitation for code outside equivclass.c to
> break the fundamental invariants of ECs --- and I also think you've taught
> appendinfo.c far more than it ought to know about the innards of ECs.
> I'd suggest putting all of that logic into equivclass.c, with a name along
> the lines of translate_equivalence_class_to_child.  That would reverse the
> dependency, in that equivclass.c would now need to call
> adjust_appendrel_attrs ... but that function is already globally exposed.

That makes sense.  I've tried implementing EC translation to occur this
way in the updated patch.

> 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?

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.

> 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.

> The business with translated_exprs and child_target_exprs in
> set_inherited_target_rel_sizes seems to be dead code --- nothing is done
> with the list.  Should that be updating childrel->reltarget?  Or is that
> now done elsewhere, and if so, why isn't the elsewhere also handling
> childrel->joininfo?

Actually, child_target_exprs simply refers to childrel->reltarget->exprs,
so modifying it modifies the latter too, but I've found that confusing
myself.  So, I have removed it.

childrel->reltarget->exprs would only contain the targetlist Vars at this
point (added by build_base_rel_tlists called by
create_inherited_target_child_root), but translated_exprs will also
contain Vars that are referenced in WHERE clauses, which have not been
added to childrel->reltarget->exprs yet.  That's what's getting added to
childrel->reltarget in this code.

> +         * Set a non-zero value here to cope with the caller's requirement
> +         * that non-dummy relations are actually not empty.  We don't try to
> 
> What caller is that?  Perhaps we should change that rather than inventing
> a bogus value here?

OK, modified relevant Asserts in the callers.  One was set_rel_size and
other set_inherited_target_rel_sizes itself (may be called recursively).

> 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.

> If you're adding fields to PlannerInfo (or pretty much any other
> planner data structure) you should update outfuncs.c to print them
> if feasible.  Also, please avoid "add new fields at the end" syndrome.
> Put them where they logically belong.  For example, if
> inh_target_child_roots has to be the same length as simple_rel_array,
> it's not just confusing for it not to be near that field, it's
> outright dangerous: it increases the risk that somebody will forget
> to manipulate both fields.

I've moved these fields around in the struct definition.  Also, I've added
unexpanded_tlist and inh_target_child_rels to _outPlannerInfo.

Attached updated patches.  0001 that I'd previously posted is no longer
included, as I said in the other email.

Thanks,
Amit