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

On 11/12/26 07:38, Kirill Reshke wrote:
> On Thu, 12 Nov 2026 at 02:48, Tomas Vondra <tomas(at)vondra(dot)me> wrote:
>>
>> Hi,
>>
>> Here's a rebased version of the patch(es). I've made no change to the
>> code itself, I've tried to improve the commit message to explain why the
>> expressions need to be null-preserving, handling of COALESCE clauses and
>> placeholders.
>
> Hi!
>
>> 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 think it is. There is find_nonnullable_rels which solves similar
> task, but I don't see any additional smartness in it.
> There is also is_standard_join_alias_expression function but it
> doesn't check for functions strictness
>
>> 2) Is is_full_join_expr_null_preserving() correct?
>
> I think it is.
>
>
> It does lose some optimizations for safe filter pushdows, because
> this doesn't recurse
> + if (IsA(node, PlaceHolderVar))
> + return ((PlaceHolderVar *) node)->phlevelsup == 0;
>

Why would that matter? That returns 'true' for all PlaceHolderVars,
treating them as null-preserving, i.e. allowing pushdown. Why would it
block the optimization in the example you posted?

AFAICS it's treating the expression as unsafe because of the "+"
operator, because the associated function is not labeled as strict.

test=# select oid, proname, proisstrict from pg_proc where oid = 177;
oid | proname | proisstrict
-----+---------+-------------
177 | int4pl | t
(1 row)

I don't think that's really the fault of this patch, and I don't see a
way around it - if there are non-strict functions, that makes the whole
expression unsafe.

I don't recall why int4pl is not labeled as strict, maybe it should be?
I haven't thought about that very much, there probably are multiple
threads in the archives discussing that. But that's not something we can
fix without a catversion bump, so it wouldn't help backbranches.

Thinking about is_full_join_expr_null_preserving() a bit more, I wonder
it needs to "recurse better" by using an expression walker. Right now
the OpExpr is treated as an opaque expression, we never even get to the
PlaceHolderVar. I'm not sure if that's a problem, though.

regards

--
Tomas Vondra