Hi,
On 2023-02-10 18:12:32 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > The problem is that normally we supress the moving aggregate optimization if a
> > volatile function is contained in the filter. But unfortunately,
> > contain_volatile_functions() doesn't descend into subplans. So we don't see
> > the volatile expression.
>
> I would say that if a volatile function in the argument crashes things,
> that's an executor bug. You won't get any sympathy from me for
> complaints about whether contain_volatile_functions noticed that,
> because *immutability markings can be lies*. It is not acceptable
> to crash if they're wrong.
This leads to wrong query results, with correctly labeled functions. I don't
have a problem with bogus results if they're not correctly labeled, but they
should be correct, if correctly labeled.
Clearly we can't crash in production builds, due to a mislabeled function. I'm
a bit more on the fence about whether assertion failures are ok.
> It looks to me like maybe we could just remove the Assert and do
>
> - if (peraggstate->transValueCount == 1)
> + if (peraggstate->transValueCount < 2)
>
> a few lines further down? I've not dug into the details though.
I'd make it an ERROR, similar to this case:
if (peraggstate->transValueIsNull)
elog(ERROR, "aggregate transition value is NULL before inverse transition");
afaics once we got to this point, the query results are always bogus. It's not
a reliable detection of mislabeled volatility, but still seems vastly better
than knowingly returning wrong results.
Greetings,
Andres Freund
