Re: [PATCH] Negative Transition Aggregate Functions (WIP)

Поиск
Список
Период
Сортировка
От David Rowley
Тема Re: [PATCH] Negative Transition Aggregate Functions (WIP)
Дата
Msg-id CAApHDvq6SOD15-jw5=PcQTe+HHOC5uQEd-4880irxrSeOKk9wQ@mail.gmail.com
обсуждение исходный текст
Ответ на Re: [PATCH] Negative Transition Aggregate Functions (WIP)  (David Rowley <dgrowleyml@gmail.com>)
Список pgsql-hackers
Дерево обсуждения
On Tue, Dec 17, 2013 at 11:06 AM, David Rowley <dgrowleyml@gmail.com> wrote:
On Tue, Dec 17, 2013 at 1:18 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Once again: this patch has no business changing any user-visible behavior.
That would include not changing the number of evaluations of volatile
functions.  The planner is full of places where optimizations are disabled
for volatile subexpressions, and I don't see why this should be different.


My point was meant to be more along the lines of that I thought it was already broken and it perhaps should be fixed or at the very least we could warn the users about it.
I would imagine that most of those other places in the planner are to prevent extra evaluations of volatile functions? In this particular case we're already evaluating these multiple extra times when a tuple moves of the top of the frame. I would have thought that we should only evaluate the volatile function once per tuple. This is not what the current implementation does.

I don't have an issue skipping this optimisation when the aggregate's expression contain any volatile functions. I just wanted to raise my concerns about the current behaviour, which I find a bit bizarre.



To improve on the example I used to try and get my point across:

These were all run on an unpatched copy of HEAD. Ignore the actual results from sum() and look at what currval() is set to after each query.

create sequence seq;
select sum(nextval('seq')) over (order by n rows between current row and unbounded following)
from generate_series(1,10) n(n);
select currval('seq');

drop sequence seq;
create sequence seq;
select sum(nextval('seq')) over (order by n)
from generate_series(1,10) n(n);
select currval('seq');

nextval() is executed 55 times with the first query and 10 times with the 2nd query. Of course this is because the current implementation requires that when a tuple moves out of scope that the whole frame be re-aggregated.
I had thought that all of the places that disabled optimisations due to there being volatile somewhere were to stop the number of executions being undefined, this case seems undefined already, or at least I can't find anywhere in the docs that says the expression will be executed this number of times. 
 
Once again, I'm not fighting to have inverse transitions uses when volatile functions are involved, I'll happily disable that. I just wanted to raise this to find out if it's intended or not and it seemed like a good thread to do it on.

Regards

David Rowley

В списке pgsql-hackers по дате отправления:

Предыдущее
От: Amit Kapila
Дата:
Сообщение: Re: Heavily modified big table bloat even in auto vacuum is running
Следующее
От: Heikki Linnakangas
Дата:
Сообщение: Re: Race condition in b-tree page deletion