Re: BUG #17777: An assert failed in nodeWindowAgg.c

Hi,

On 2023-02-10 18:12:32 -0500, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)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