One thing that is currently on my mind is what to do when passing volatile functions to the aggregate. Since the number of times we execute a volatile function will much depend on the window frame options, I think we should include some sort of warning in the documentation that "The number of times that the expression is evaluated within the aggregate function when used in the context of a WINDOW is undefined". The other option would be to disable this optimisation if the aggregate expression contained a volatile function, but doing this, to me seems a bit weird as is anyone actually going to be depending on a volatile function being executed so many times?
I just wanted to bring this back into people's minds.
During writing this patch I found and removed a comment which was a todo item to implement these negative transition functions. This comment said about maybe disallowing the use of these if the expression of the function contained a volatile function. I wondered why this was important and I still don't really see why we would disallow this only to enforce that we call that function an undefined number of times anyway.
nextval was the only volatile function that I could think of that would allow me to give an example which was easy to understand what is going on here.
CREATE SEQUENCE test_seq;
SELECT currval('test_seq'),
COUNT(*) OVER (ORDER BY x.x ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING),
SUM(nextval('test_seq')) OVER (ORDER BY x.x ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING)
FROM generate_series(1,2) x (x);
DROP SEQUENCE test_seq;
The results are:
currval | count | sum
---------+-------+-----
2 | 2 | 3
3 | 1 | 3
I've not looked to see if the spec has anything about this but, the first row will have sum as 1+2 then the 2nd row will just have 1 row to aggregate and the value will be 3 due to nextval returning 3, I could see an argument that the results for this should actually be:
currval | count | sum
---------+-------+-----
2 | 2 | 3
3 | 1 | 2
If it was then the solution would have to be to materalise the expression by evaluating it once for each tuple which sounds like a big change. I thought maybe if we're going to be playing around with the number of times these expressions are evaluated then we should stick a node in the docs to tell our users not to depend on this.
