| 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 |
| 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 |