Re: reconsider_full_join_clause fails to consider if COALESCE() arguments are NULL-preserving

On 11/9/26 06:47, Ayush Tiwari wrote:
> Hi,
>
> On Mon, 9 Nov 2026 at 02:37, Tomas Vondra <tomas(at)vondra(dot)me
> <mailto:tomas(at)vondra(dot)me>> wrote:
>>
>> Hi,
>>
>> While working on [1], an AI review pointed out a separate long-standing
>> bug in reconsider_full_join_clause(). Consider a query like this:
>>
>>     SELECT *
>>     FROM t1 FULL JOIN t2 ON (t1.id <http://t1.id> = t2.id <http://t2.id>)
>>     WHERE COALESCE(t1.id <http://t1.id>, t2.id <http://t2.id>) = 12345
>>
>> The function correctly replaces the COALESCE() clause with two clauses
>> pushed down to the join inputs, and removes the join clause, as if
>>
>>     SELECT *
>>     FROM (SELECT * FROM t1 WHERE t1.id <http://t1.id> = 12345)
>>     FULL JOIN (SELECT * FROM t2 WHERE t2.id <http://t2.id> = 12345)
>>
>> So far all good - such transformation is correct (this example does not
>> even need fixes from [1]). But the coalesce arguments may not be plain
>> Vars. Consider this instead:
>>
>>     SELECT *
>>     FROM t1 FULL JOIN t2 ON (t1.id <http://t1.id> = COALESCE(t2.id
> <http://t2.id>, 12345))
>>     WHERE COALESCE(t1.id <http://t1.id>, COALESCE(t2.id <http://
> t2.id>, 12345)) = 12345
>>
>> Well, reconsider_full_join_clause() still pushed down the clauses to the
>> join inputs etc. as if
>>
>>     SELECT *
>>     FROM (SELECT * FROM t1 WHERE t1.id <http://t1.id> = 12345)
>>     FULL JOIN (SELECT * FROM t2 WHERE COALESCE(t2.id <http://
> t2.id>,12345) = 12345)
>>
>> But this is wrong, because the COALESCE() gets rid of NULLs. If t2.id
> <http://t2.id> is
>> NULL ("id" does not need to be a PK), the row incorrectly matches. A
>> trivial example:
>>
>>     CREATE TABLE t1 (id INT);
>>     CREATE TABLE t2 (id INT);
>>
>>     INSERT INTO t1 VALUES (NULL);
>>     INSERT INTO t2 VALUES (NULL);
>>
>>     SELECT t1.id <http://t1.id> as x, t2.id <http://t2.id> as y
>>       FROM t1 FULL JOIN t2 ON (t1.id <http://t1.id> = COALESCE(t2.id
> <http://t2.id>, 12345))
>>      WHERE COALESCE(t1.id <http://t1.id>, COALESCE(t2.id <http://
> t2.id>, 12345)) = 12345;
>>
>>      x | y
>>     ---+---
>>        |
>>     (1 row)
>>
>>     -- OFFSET 0 to block the pushdown, result should not change
>>     SELECT * FROM (
>>      SELECT t1.id <http://t1.id> as x, t2.id <http://t2.id> as y
>>        FROM t1 FULL JOIN t2 ON (t1.id <http://t1.id> = COALESCE(t2.id
> <http://t2.id>, 12345)) OFFSET 0
>>     ) foo WHERE COALESCE(foo.x, COALESCE(foo.y, 12345)) = 12345;
>>
>>      x | y
>>     ---+---
>>        |
>>        |
>>     (2 rows)
>>
>> COALESCE is not the only way to get this issue, CASE can have the same
>> effect (example in the patch). There might be more ways to hit it ...
>>
>> This requires the t1.id <http://t1.id> = COALESCE(t2.id <http://
> t2.id>, 12345) join clause, it works
>> OK without that. IIUC this is because it establishes a matching EC,
>> which is necessary for the misbehavior.
>
> Thanks for the detailed explanation.  This is almost the first time I
> didn't have to go and search for the whole issue to understand what the
> case is about.
>

TBH this is a very new topic for me too, so I mostly wrote this
explanation for myself in the first place, to clarify my understanding
of how it works. And indeed I found a bunch of gaps in my understanding.
But it also means the explanation may be wrong / missing pieces, so I'd
encourage you to go through the code and verify I got it right - it'd be
a very useful review.

>>
>> This query pattern (with COALESCE in the join clause) is pretty odd, but
>> what do I know? People (and tools) sometimes write pretty crazy stuff.
>>
>>
>> AFAICS to fix this, reconsider_full_join_clause() needs to ask if both
>> sides of the clause (or operands for COALESCE) preserve NULLs. The patch
>> adds that check before the loop, same place as [1]. The principle is the
>> same - unsafe expression -> return false -> keep the original clause.
>>
>> The trouble is, I don't think we have a way to check this property yet.
>> There are places doing something similar, but nothing that'd check if an
>> expression is null-preserving (recursively).
>>
>> After a fair amount of trial and error I came up with a new helper
>> is_full_join_expr_null_preserving doing this. It pretty much descends
>> into CoalesceExpr, and checks if the arguments are NULL-preserving.
>>
>> Still, once we get a non-CoalesceExpr expression, we need to know if
>> it's null-preserving. The conservative way to do that is
>>
>>     return !contain_nonstrict_functions(node) &&
>>         contain_vars_of_level(node, 0);
>>
>> Which means it may contain only strict functions (NULL -> NULL) and some
>> vars. This rules out constants, functions that discard NULLs, etc. It
>> also rejects strict-behaving functions that are not labeled as such, but
>> I don't know how to do better.
>>
>> There's also a special case for PlaceHolderVar, to preserve the
>> optimization for expressions calculated below the join (as the result
>> will be nulled no matter the expression).
>>
>> Maybe we could improve this further, but this query pattern is pretty
>> strange, so it should be enough to not return incorrect results. I don't
>> think it's worth more effort.
>
> +1, Ig being conservative is fine here.  If there's a report for some real
> query that needs more, we can teach it then.
>
>> The patch is pretty small. I've spent a bit of time with sqlfiddle to
>> check we get the same behavior as other databases (MSSQL and Oracle).
>> And that we produce incorrect results without the fix.
>>
>> My questions are pretty much:
>>
>> 1) Is is_full_join_expr_null_preserving() necessary? Or is there some
>> existing piece of code checking the expression preserves NULLs? Possibly
>> with some additional smartness I haven't thought of?
>
> I don't think we have one.  I looked at find_nonnullable_rels() first,
> but FALSE is good enough for that one, and here we need an actual NULL.
> So, yeah the function looks necessary to me.
>
>> 2) Is is_full_join_expr_null_preserving() correct?
>
> Looks right to me.  If all the COALESCE args go NULL, so does the
> COALESCE.  The PHV case is fine too, since it's the value computed below
> the join that gets nulled.  IIUC that part relies on the side check from
> [1] though, so I assume this will go on top of it?

OK

>
> Regards,
> Ayush
>
> PS. I did trip over the JsonExpr strictness bug while testing (a NULL
> PASSING arg can still give you a non-NULL result), but that's already
> being fixed in [2], so nothing to do here.
>

Ah, interesting.

Thanks!

--
Tomas Vondra