Re: remove_useless_joins vs. bug #19560

On Tue, Jul 28, 2026 at 3:21 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> So attached are a set of patches along that line. v5-0001 is the
> master patchset squashed to one patch; it differs from the v4 series
> in having the remove_useless_joins rename and some minor additional
> comment-smithing.

I reviewed v5-0001, and here are some comments.

1. The initialization of all_result_relids/leaf_result_relids is moved
to below the trivial-jointree early return. So for a query like
"INSERT INTO t VALUES()", both sets now stay NULL where they used to
be {resultRelation}.

I'm not sure if this can cause any issues, but at least it contradicts
the comment in pathnodes.h:

* all_result_relids is empty for SELECT, otherwise it contains at least
* parse->resultRelation.

2. Regarding remove_rels_from_query_tree(): I complained about this
function in my previous email, but it does not get changed.

First, I think we can at least add a check on root->glob->lastPHId and
do the ChangeVarNodes walks only if there are any PHVs in the query.
I believe only PHVs can still contain references to the removed relids
here.

Second, as you mentioned, the comment claims that "the relids can
still appear in the nullingrels sets of surviving Vars", but that is
not true. We should not keep such comment as it is very misleading.

Third, this function works only if "There should be no ordinary Vars
of a removed relation left". I agree it holds today. But if it ever
does break, ChangeVarNodes sets var->varno = -1, which is INNER_VAR,
and AppendRelInfo->parent_relid/child_relid would go to -1 too. This
would produce garbage rather than an error, which makes me nervous.
I hope we can use a walker that mutates on PHVs only.

3. In reduce_unique_semijoins(), analyzejoins.c:480 tests
min_righthand for singleton-ness, but analyzejoins.c:517 uses
syn_righthand to find the JoinExpr and flips the whole node to
JOIN_INNER. If there is a case where min_righthand is singleton but
syn_righthand is not and the innerrel is unique, we will be in
trouble.

4. The commit message claims that "it doesn't seem to result in any
significant planning-time penalty". I'm somewhat skeptical of that.

- Richard