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

From: Tomas Vondra <tomas(at)vondra(dot)me>
To: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Two bugs in reconsider_full_join_clause(), handling coalesce() = const clauses
Date: 2026-11-07 23:01:57
Message-ID: d3453684-511c-4bc6-bbaa-66b59cee1ae3@vondra.me
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

Here's a much more polished version of the patches, with per-branch
patches. I've significantly shortened the commit message, expanded the
tests a bit to test additional variations, etc.

The 16-master code is the same, there are just some minor plan changes
in tests. The 15 is more different, as it does not have the JoinDomain
stuff, so it needs to fix just one of the bugs.

After all this cleanup and more testing, I feel much more confident in
correctness of the patches. It's not a very complex fix, so I intend to
commit not to long after the minor releases are out.

I'd still welcome a review and feedback, of course. One thing I'm not
entirely sure about is the lookup of SpecialJoinInfo in the fix for 15
(in reconsider_full_join_clause).

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.

regards

--
Tomas Vondra

Attachment Content-Type Size
v2-master-0001-Fix-constant-propagation-through-FULL-JOIN.patch text/x-patch 16.8 KB
v2-19-0001-Fix-constant-propagation-through-FULL-JOIN-cla.patch text/x-patch 16.8 KB
v2-18-0001-Fix-constant-propagation-through-FULL-JOIN-cla.patch text/x-patch 16.7 KB
v2-17-0001-Fix-constant-propagation-through-FULL-JOIN-cla.patch text/x-patch 16.7 KB
v2-15-0001-Fix-constant-propagation-through-FULL-JOIN-cla.patch text/x-patch 9.7 KB
v2-16-0001-Fix-constant-propagation-through-FULL-JOIN-cla.patch text/x-patch 16.7 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Previous Message Jan Bjørge Løvland 2026-11-07 22:00:19 Re: Design proposal: grain, exact per-time-bucket aggregates from a summarizing index