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

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.

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.

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?

2) Is is_full_join_expr_null_preserving() correct?

regards

[1] https://postgr.es/m/92808d26-1b4a-48fe-82f0-597da3ff471f@vondra.me

--
Tomas Vondra