Re: remove_useless_joins vs. bug #19560

Richard Guo <guofenglinux(at)gmail(dot)com> writes:
> I reviewed v6 as a whole and it LGTM. I have a few small changes to
> suggest, see the attached delta on top of v6.

> * The first one is to initialize all_result_relids/leaf_result_relids
> in subquery_planner like we did before. v5 moved that into
> query_planner, but as far as I can tell the only reason was the Assert
> in the old self-join code, which is gone now. And I think the current
> initialization can contradict the comment in pathnodes.h, as I
> mentioned in [1]:point 1.

I don't like this on the grounds of consistency. As things stand now,
we partially initialize those fields in subquery_planner and then
finish filling them in much later, during add_other_rels_to_query.
What I did in my proposed patch was to collect all of that processing
together, in successive steps near the end of query_planner. This
makes it clear that those fields are not to be relied on before that
point, and in particular that they don't need to be reset in the
restart loop in the initial part of query_planner. If we change it
back as you suggest, then it'll be really nonobvious why this derived
data is handled differently from other derived data. And the fact
that it's only partially valid during the restart loop seems to me
to be a bug hazard: if someone tried to rely on these fields in
logic within that loop, it might seem to work as long as they'd not
tested it on inheritance/partitioning cases. So I think moving this
logic is good cleanup, albeit maybe not strictly necessary to the
immediate problem.

I take your point that the early-exit-for-trivial-jointree path now
fails to set these fields at all, but I'd rather handle that by
adding a couple more lines in that path to fill them in. Yup, it'd
be duplicate logic, but it seems cleaner that way.

> * The Assert on resultRelation in remove_useless_outer_joins can be
> dropped, because join_is_removable already rejects that case.

Well, yeah, but on that argument we could drop the entire stanza,
because it's just checking that join_is_removable didn't mess up.
Maybe we should? I've not heard that anybody ever hit those Asserts.

Your other changes look good.

regards, tom lane