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

Hi,

On Mon, 9 Nov 2026 at 02:37, Tomas Vondra <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 = t2.id)
> WHERE COALESCE(t1.id, 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 = 12345)
> FULL JOIN (SELECT * FROM t2 WHERE 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 = COALESCE(t2.id, 12345))
> WHERE COALESCE(t1.id, COALESCE(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 = 12345)
> FULL JOIN (SELECT * FROM t2 WHERE COALESCE(t2.id,12345) = 12345)
>
> But this is wrong, because the COALESCE() gets rid of NULLs. If 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 as x, t2.id as y
> FROM t1 FULL JOIN t2 ON (t1.id = COALESCE(t2.id, 12345))
> WHERE COALESCE(t1.id, COALESCE(t2.id, 12345)) = 12345;
>
> x | y
> ---+---
> |
> (1 row)
>
> -- OFFSET 0 to block the pushdown, result should not change
> SELECT * FROM (
> SELECT t1.id as x, t2.id as y
> FROM t1 FULL JOIN t2 ON (t1.id = COALESCE(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 = COALESCE(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.

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

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.

[1] https://postgr.es/m/92808d26-1b4a-48fe-82f0-597da3ff471f@vondra.me
[2]
https://www.postgresql.org/message-id/CAD=-kXbsgddZeUJKDi5=-rE4pHdvNFYWk0FcE85MCzN4gzo0dQ@mail.gmail.com