Re: Two bugs in reconsider_full_join_clause(), handling coalesce() = const clauses

On 11/8/26 00:01, Tomas Vondra wrote:
> ...
>
> I'm wondering about these things:
>
> 1) Is there a better way to find the right join for the clause than
> walking root->join_info_list? I haven't found any. In 16 and later
> reconsider_full_join_clause gets it from the outside through the
> OuterJoinClauseInfo. I believe that's not possible in 15.
>
> 2) Let's say we need to do the lookup in root->join_info_list. Is the
> condition used to "match" the join correct?
>
> bms_equal(rinfo->outer_relids,
> bms_union(cur_sjinfo->syn_lefthand,
> cur_sjinfo->syn_righthand)))
>
> Or could it be wrong in some corner cases? AFAIK it's correct, but maybe
> there's a weird corner case that breaks it.
>
> 3) I'm not sure about the elog(ERROR) after the lookup:
>
> if (sjinfo == NULL)
> elog(ERROR, "failed to find SpecialJoinInfo for FULL JOIN clause");
>
> Maybe it should be replaced with an assert - we're not really expecting
> a failure here, right? We should always find the SpecialJoinInfo. But if
> the condition in (2) may not find the right join, maybe it'd be better
> to return false here, as if the condition can't be propagated. Otherwise
> a production build would segfault on the next block that dereferences
> the sjinfo pointer.
>
> So I'm thinking about replacing it with something like
>
> /* failed to find SpecialJoinInfo for FULL JOIN clause */
> if (sjinfo == NULL)
> {
> Assert(false);
> return false;
> }
>
> Suggestions welcome, ofc.
>

I've now pushed and backpatched this. I've managed to convince myself
that the 15 fix is correct, and the (sjinfo == NULL) check is not
actually necessary. I've replaced it with a plain assert().

The argument is fairly simple - I considered explaining it in a comment,
but decided not to do that. So I'll at least post it here:

When deconstruct_recurse() builds the join tree, the JOIN_FULL does:

case JOIN_FULL:
leftjoinlist = deconstruct_recurse(root, j->larg,
true,
&leftids, &left_inners,
&child_postponed_quals);
rightjoinlist = deconstruct_recurse(root, j->rarg,
true,
&rightids, &right_inners,
&child_postponed_quals);
*qualscope = bms_union(leftids, rightids);
*inner_join_rels = bms_union(left_inners, right_inners);
/* each side is both outer and inner */
nonnullable_rels = *qualscope;
nullable_rels = *qualscope;
break;

and then

sjinfo = make_outerjoininfo(root,
leftids, rightids,
*inner_join_rels,
j->jointype,
my_quals);

Which then sets leftids/rightids as syn_lefthand/syn_righthand.
distribute_qual_to_rels then uses nonnullable_rels as outer_relids. And
nonnullable_rels was calculated as bms_union(leftids, rightids).

This exactly matches the bms_equal() condition in the lookup. Also,
there really ought to be the SJI in the list, because all of this runs
from reconsider_outer_join_clauses(), right after the join tree is built
by deconstruct_jointree().

regards

--
Tomas Vondra