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

On 11/12/26 10:22, Tomas Vondra wrote:
>
> 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.
>

Ah, damn it. The int4pl *is* strict of course (as obvious from the
output I myself posted). ENOCAFFEINE.

It does indeed descend into the contains_nonstrict_functions(), which
descends into the PlaceHolderVar, and finds the COALESCE(a.x, 0) wrapped
in it, which is not strict.

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

I suppose an expression walker that does not descend into PlaceHolderVar
nodes (with phlevelsup = 0) would make this work correctly.

regards

--
Tomas Vondra