Re: speeding up planning with partitions

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

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.

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?

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.

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?

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

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.

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.

			regards, tom lane