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

On Mon, 13 Feb 2023 at 13:46, Andres Freund <andres@anarazel.de> wrote:
> What about find_compatible_agg()? I don't think there's as severe
> consequences, but it also doesn't seem right as-is.

This does not seem related to the issue being reported here. It seems
to me this would only affect the aggregate deduplication logic and
nothing else.

I agree it would be nice if we were more consistent with when volatile
functions are evaluated and when they are not. There are also a bunch
of weird issues with SRFs as I highlighted in [1].

The following shows another case where we change the volatile function
evaluation. This one we do on purpose, which might or might not be
good, depending on if you wanted the random() in the targetlist for
the same purpose as the random() in the ORDER BY.

# select random(), random(), random();
       random        |       random        |       random
---------------------+---------------------+--------------------
 0.15295030404849452 | 0.34841025373624745 | 0.7195551260981239

# select random(), random(), random() order by random();
       random       |       random       |       random
--------------------+--------------------+--------------------
 0.8569677358482639 | 0.8569677358482639 | 0.8569677358482639

It was just last week I wrote a query to fetch a random row from a
table by using ORDER BY random() LIMIT 1;  I also needed to call
random() somewhere else in the query for something unrelated but this
deduplication behaviour caused that not to work the way I wanted it
to. This caused my unrelated random() value to be consistently very
low due to it being shared with the random() for the sort. I had to
add a subquery for the 2nd random() to fix the problem.

For the case you're complaining about, I don't think shifting the
goalposts randomly in back-branches is a good idea.  For the one you
reported we might upset someone who has come to rely on the aggstate
deduplication either for performance or for the number of volatile
function evaluations. I'm not really on board with changing that
unless someone highlights that it's causing an actual problem.

David

[1] https://postgr.es/m/CAApHDvqKO00nNQtchBs65VTfjEMUsYiB5r2P4VUreTdXE9RY1g@mail.gmail.com