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

On 11/12/26 16:40, Tomas Vondra wrote:
> On 11/12/26 16:18, Kirill Reshke wrote:
>> On Thu, 12 Nov 2026 at 17:57, Tomas Vondra <tomas(at)vondra(dot)me> wrote:
>>>
>>> On 11/12/26 11:28, Tomas Vondra wrote:
>>>>
>>>> ...
>>>>
>>>> I suppose an expression walker that does not descend into PlaceHolderVar
>>>> nodes (with phlevelsup = 0) would make this work correctly.
>>>>
>>> Here's v3 doing it that way.
>>>
>>> --
>>> Tomas Vondra
>>
>> v3 looks totally fine for me, perf regressions from my report
>> eliminated - good.
>> There are maybe some more cases to recurce, but we shouldn't defer
>> this bugs fix commit because of this. LGTM
>>
>
> True. I picked only a subset of nodes for v3, but I suspect it might be
> expanded to the same set of nodes as contain_nonstrict_functions_walker
> and check_functions_in_node. I don't know if all of them can appear in
> the kind of quals we're processing.
>
> This kind of clauses is not very common, so I was conservative. But it's
> not great to cause a regression either ... I have to think about it.
>

I kept thinking about this, so I gave the expression walker idea a go,
and I like it. The master/15 patches are attached.

The new walker mostly mimics contain_nonstrict_functions_walker() with
some small tweaks for the interesting nodes (as in the previous patch).
I ended up moving it to clauses.c, for two reasons. It makes it easier
to keep the two walkers in sync when they're right next to each other.
It also means we don't need to export e.g. is_strict_saop (which is now
static in clauses.c).

I like this version more than v3. The patch is only a little bit larger,
and I trust this "copy of existing walker" more than I trust my earlier
limited implementation.

I'm now fairly happy with the patch. I'm considering whether to push it
now before the 19 freeze ...

regards

--
Tomas Vondra