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

From: Kirill Reshke <reshkekirill(at)gmail(dot)com>
To: Tomas Vondra <tomas(at)vondra(dot)me>
Cc: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Two bugs in reconsider_full_join_clause(), handling coalesce() = const clauses
Date: 2026-11-11 18:49:37
Message-ID: CALdSSPjkSLnD93bjnOgnQudcK2CsMnkPZMjRiVmQMd_eNBskYA@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Wed, 11 Nov 2026 at 20:54, Tomas Vondra <tomas(at)vondra(dot)me> wrote:
>
> 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
>

Hi!
Cool findings.

There is another issue I found during review, but this issue is not
directly related (exist on master before and after
e50d958f092bc33fcb51089c68c72779bae2a6a3), and maybe should be fixed
as separate bugs.But I decided to report here

example setup & query

CREATE TABLE fj_left (x int);
CREATE TABLE fj_right (x int);
INSERT INTO fj_left VALUES (0);
INSERT INTO fj_right VALUES (1);

SELECT a.x AS ax, b.x AS bx
FROM fj_left a FULL JOIN fj_right b ON concat(a.x) = b.x::text
WHERE coalesce(concat(a.x), b.x::text) = '';

(zero rows)

The thing is reconsider_full_join_clause() assumes that a COALESCE
operand becomes NULL its input is. That does not hold for nonstrict
functions like concat(), num_nulls, format, etc.

If disable full join predicate consideration with OFFSET 0, query
behaves correctly.

reshke=# SELECT * FROM (
SELECT a.x AS ax, b.x AS bx
FROM fj_left a FULL JOIN fj_right b ON concat(a.x) = b.x::text OFFSET 0
) s WHERE coalesce(concat(ax), bx::text) = '';
ax | bx
----+----
| 1
(1 row)

another example:

SELECT a.x AS ax, b.x AS bx
FROM fj_left a FULL JOIN fj_right b ON quote_nullable(a.x) = b.x::text
WHERE coalesce(quote_nullable(a.x), b.x::text) = 'NULL';
(0 rows, 1 row with OFFSET 0 hack)

--
Best regards,
Kirill Reshke

Attachment Content-Type Size
v1-master-0001-Reject-nonstrict-FULL-JOIN-deductions.patch application/octet-stream 978 bytes

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Matheus Alcantara 2026-11-11 18:54:24 Re: extension_control_path: ignore empty elements and deduplicate directories
Previous Message Masahiko Sawada 2026-11-11 18:40:57 Re: Pass heap tuple counts to parallel vacuum as double