Amit Langote <langote_amit...@lab.ntt.co.jp> writes:
> On 2019/02/20 5:57, Tom Lane wrote:
>> but that is not
>> going to fix the fundamental problem: the cost estimate for such a
>> Path should vary depending on how large we think the outer rel is,
>> and we don't have a reasonable way to set that if we're trying to
>> make a one-size-fits-all Path for something that's being joined to
>> an inheritance tree with a widely varying set of relation sizes.
> What if we set the parent target relation's rows to an average of child
> target relation's rows, that is, instead of setting it to dummy 1 that
> previous versions of the patches were doing?
Well, if somebody were holding a gun to our collective heads and saying
you must do inherited UPDATE/DELETE this way, we could probably limp along
with that; or maybe it'd be better to use the sum of the children's row
counts. That depends on how many of the per-child join plans end up using
the parameterized path, which is something we couldn't hope to guess so
early. (Arguably, the way the code is now, it's overestimating the true
costs of such paths, since it doesn't account for different child plans
possibly using the same indexscan and thereby getting caching benefits.)
In any case there'd be side-effects on code that currently expects
appendrels to have size zero, eg the total_table_pages calculation in
make_one_rel.
However, there are other reasons why I'm not really happy with the
approach proposed in this patch.
The main problem is that cloning the PlannerInfo while still sharing a lot
of infrastructure between the clones is a horrid hack that I think will be
very buggy and unmaintainable. We've gotten away with it so far in
inheritance_planner because (a) the complexity is all local to that
function and (b) the cloning happens very early in the planning process,
so that there's not much shared subsidiary data to worry about; mostly
just the parse tree, which in fact isn't shared because the first thing
we do is push it through adjust_appendrel_attrs. This patch proposes
to clone much later, and both the actual cloning and the consequences
of that are spread all over, and I don't think we're nearly done with
the consequences :-(. I found the parameterized-path problem while
wondering why it was okay to not worry about adjusting attrs in data
structures used during path construction for other baserels ... turns
out it isn't. There's a lot of other stuff in PlannerInfo that you're
not touching, for instance pathkeys and placeholders; and I'm afraid
much of that represents either current bugs or future problems.
So what I feel we should do is set this aside for now and see if we
can make something of the other idea I proposed. If we could get
rid of expand-inheritance-at-the-top altogether, and plan inherited
UPDATE/DELETE the same as inherited SELECT, that would be a large
reduction in planner complexity, hence much to be preferred over this
approach which is a large increase in planner complexity. If that
approach crashes and burns, we can come back to this.
There might be parts of this work we can salvage, though. It seems
like the idea of postponing expand_inherited_tables() might be
something we could use anyway.
regards, tom lane
