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

2014-04-14 Thread Robert Haas
On Sun, Apr 13, 2014 at 2:23 PM, Tom Lane t...@sss.pgh.pa.us wrote: * I also left out the table documenting which aggregates have this optimization. That's not the kind of thing we ordinarily document, and it seems inevitable to me that such a table would be noteworthy mostly for

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

2014-04-13 Thread Tom Lane
Dean Rasheed dean.a.rash...@gmail.com writes: OK, I'm marking this ready for committer attention, on the understanding that that doesn't include the invtrans_minmax patch. I've finished reviewing/revising/committing this submission. Some notes: * I left out the EXPLAIN ANALYZE statistics, as I

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

2014-04-13 Thread Florian Pflug
On Apr13, 2014, at 20:23 , Tom Lane t...@sss.pgh.pa.us wrote: Dean Rasheed dean.a.rash...@gmail.com writes: OK, I'm marking this ready for committer attention, on the understanding that that doesn't include the invtrans_minmax patch. I've finished reviewing/revising/committing this

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

2014-04-13 Thread David Rowley
-Original Message- From: Tom Lane [mailto:t...@sss.pgh.pa.us] Sent: 14 April 2014 06:23 To: Dean Rasheed Cc: Florian Pflug; David Rowley; Kevin Grittner; Josh Berkus; Greg Stark; PostgreSQL-development Subject: Re: [HACKERS] [PATCH] Negative Transition Aggregate Functions (WIP

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

2014-04-11 Thread Dean Rasheed
On 10 April 2014 22:52, Tom Lane t...@sss.pgh.pa.us wrote: Dean Rasheed dean.a.rash...@gmail.com writes: I was imagining that firsttrans would only be passed the first value to be aggregated, not any previous state, and that it would be illegal to specify both an initcond and a firsttrans

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

2014-04-11 Thread Tom Lane
Florian Pflug f...@phlo.org writes: Which is why I feel that having two separate sets of transition functions and state types solves the wrong problem. If we find a way to prevent transition functions from being called directly, we'll shave a few cycles of quite a few existing aggregates,

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

2014-04-11 Thread Florian Pflug
On Apr11, 2014, at 17:09 , Tom Lane t...@sss.pgh.pa.us wrote: Basically, this comes down to a design judgment call, and my judgment is differing from yours. In the absence of opinions from others, I'm going to do it my way. Ok. Are you going to do the necessary changes, or shall I? I'm happy

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

2014-04-11 Thread Tom Lane
Florian Pflug f...@phlo.org writes: On Apr11, 2014, at 17:09 , Tom Lane t...@sss.pgh.pa.us wrote: Basically, this comes down to a design judgment call, and my judgment is differing from yours. In the absence of opinions from others, I'm going to do it my way. Ok. Are you going to do the

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

2014-04-11 Thread Florian Pflug
On Apr11, 2014, at 19:04 , Tom Lane t...@sss.pgh.pa.us wrote: It strikes me that your second point 2) It allows strict aggregates whose state type and input type aren't binary coercible to return NULL if all inputs were NULL without any special coding. As it stands today, this

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

2014-04-11 Thread Tom Lane
Dean Rasheed dean.a.rash...@gmail.com writes: On 10 April 2014 19:54, Tom Lane t...@sss.pgh.pa.us wrote: So if we go with that terminology, perhaps these names for the new CREATE AGGREGATE parameters: initfuncapplies to plain aggregation, mutually exclusive with initcond msfunc

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

2014-04-11 Thread Tom Lane
Florian Pflug f...@phlo.org writes: Yes, the idea had come up at some point during the review discussion. I agree that it's only worthwhile if it works for state type internal - though I think there ought to be a way to allow that. We could for example simply decree that the initfunc's first

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

2014-04-11 Thread Florian Pflug
On Apr11, 2014, at 19:42 , Tom Lane t...@sss.pgh.pa.us wrote: Florian Pflug f...@phlo.org writes: Yes, the idea had come up at some point during the review discussion. I agree that it's only worthwhile if it works for state type internal - though I think there ought to be a way to allow that.

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

2014-04-10 Thread David Rowley
On Thu, Apr 10, 2014 at 9:55 AM, Tom Lane t...@sss.pgh.pa.us wrote: I'm pretty sure David Rowley did some benchmarking. The results should be in this thread somewhere I think, but they currently evade me... Maybe David can re-post, if he's following this... I saw benchmarks addressing

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

2014-04-10 Thread David Rowley
On Thu, Apr 10, 2014 at 9:55 AM, Tom Lane t...@sss.pgh.pa.us wrote: Florian Pflug f...@phlo.org writes: I was (and still am) not in favour of duplicating the whole quadruple of (state, initialvalue, transferfunction, finalfunction) because it seems excessive. In fact, I believed that doing

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

2014-04-10 Thread Tom Lane
Dean Rasheed dean.a.rash...@gmail.com writes: On 10 April 2014 01:13, Florian Pflug f...@phlo.org wrote: However, I still believe the best approach at this point is to just work on making int4_avg_accum faster. I still see no principal reason what it has to be noticeably slower - the only

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

2014-04-10 Thread Dean Rasheed
On 10 April 2014 15:18, Tom Lane t...@sss.pgh.pa.us wrote: Dean Rasheed dean.a.rash...@gmail.com writes: On 10 April 2014 01:13, Florian Pflug f...@phlo.org wrote: However, I still believe the best approach at this point is to just work on making int4_avg_accum faster. I still see no principal

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

2014-04-10 Thread Tom Lane
Dean Rasheed dean.a.rash...@gmail.com writes: On 10 April 2014 15:18, Tom Lane t...@sss.pgh.pa.us wrote: This idea of a separate firsttrans function is interesting but perhaps orthogonal to the current patch. Also, I don't quite understand how it would work for aggregates with null

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

2014-04-10 Thread Dean Rasheed
On 10 April 2014 19:04, Tom Lane t...@sss.pgh.pa.us wrote: Dean Rasheed dean.a.rash...@gmail.com writes: On 10 April 2014 15:18, Tom Lane t...@sss.pgh.pa.us wrote: This idea of a separate firsttrans function is interesting but perhaps orthogonal to the current patch. Also, I don't quite

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

2014-04-10 Thread Tom Lane
Dean Rasheed dean.a.rash...@gmail.com writes: On 10 April 2014 19:04, Tom Lane t...@sss.pgh.pa.us wrote: What about names for the invertible-aggregate infrastructure? I'm tempted to prefix inv to all the existing names, but then invsfunc means the alternate forward function ... can we use

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

2014-04-10 Thread Dean Rasheed
On 10 April 2014 19:54, Tom Lane t...@sss.pgh.pa.us wrote: Dean Rasheed dean.a.rash...@gmail.com writes: On 10 April 2014 19:04, Tom Lane t...@sss.pgh.pa.us wrote: What about names for the invertible-aggregate infrastructure? I'm tempted to prefix inv to all the existing names, but then

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

2014-04-10 Thread Florian Pflug
On Apr10, 2014, at 02:13 , Florian Pflug f...@phlo.org wrote: On Apr9, 2014, at 23:17 , Florian Pflug f...@phlo.org wrote: On Apr9, 2014, at 21:35 , Tom Lane t...@sss.pgh.pa.us wrote: A quick test says that avg(int4) is about five percent slower than sum(int4), so that's the kind of hit we'd

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

2014-04-10 Thread Tom Lane
Dean Rasheed dean.a.rash...@gmail.com writes: I was imagining that firsttrans would only be passed the first value to be aggregated, not any previous state, and that it would be illegal to specify both an initcond and a firsttrans function. The forward transition function would only be called

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

2014-04-10 Thread Florian Pflug
On Apr10, 2014, at 21:34 , Dean Rasheed dean.a.rash...@gmail.com wrote: On 10 April 2014 19:54, Tom Lane t...@sss.pgh.pa.us wrote: So if we go with that terminology, perhaps these names for the new CREATE AGGREGATE parameters: initfuncapplies to plain aggregation, mutually exclusive

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

2014-04-10 Thread Tom Lane
Florian Pflug f...@phlo.org writes: I still think you're getting ahead of yourselves here. The number of aggregates which benefit from this is tiny SUM(int2,int4) and maybe BOOL_{AND,OR}. And in the SUM(int2,int4) case *only* on 64-bit archs - for the others, the state type is already

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

2014-04-10 Thread Florian Pflug
On Apr11, 2014, at 00:07 , Tom Lane t...@sss.pgh.pa.us wrote: Florian Pflug f...@phlo.org writes: I still think you're getting ahead of yourselves here. The number of aggregates which benefit from this is tiny SUM(int2,int4) and maybe BOOL_{AND,OR}. And in the SUM(int2,int4) case *only* on

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

2014-04-10 Thread Tom Lane
Florian Pflug f...@phlo.org writes: My argument is that is costs us more complexity to duplicate everything for the invertible case, *and* the result seems less flexible - not from the POV of aggregate implementations, but from the POV of future extensions. [ shrug... ] You can argue against

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

2014-04-10 Thread Florian Pflug
On Apr11, 2014, at 01:30 , Tom Lane t...@sss.pgh.pa.us wrote: Florian Pflug f...@phlo.org writes: As for evidence - have you looked at the patch I posted? I'd be very interested to know if it removes the performance differences you saw. (1) You can't really prove the absence of a performance

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

2014-04-09 Thread Dean Rasheed
On 8 April 2014 21:48, Florian Pflug f...@phlo.org wrote: On Apr7, 2014, at 17:41 , Dean Rasheed dean.a.rash...@gmail.com wrote: I've just finished reading through all the other patches, and they all look OK to me. It's mostly straightforward stuff, so despite the size it's hopefully all

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

2014-04-09 Thread Tom Lane
Dean Rasheed dean.a.rash...@gmail.com writes: OK, I'm marking this ready for committer attention, on the understanding that that doesn't include the invtrans_minmax patch. I've started to look at this patch set. After rereading the thread, I'm thinking that it's a mistake to just add the

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

2014-04-09 Thread Tom Lane
I wrote: ... an invertible aggregate may require a more complex transition state data structure --- in particular, if you're forced to go from a pass-by-value to a pass-by-reference data type, right there you are going to take a big hit in aggregate performance, and there is no way for the

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

2014-04-09 Thread Florian Pflug
On Apr9, 2014, at 21:35 , Tom Lane t...@sss.pgh.pa.us wrote: As a quick check, I compared aggregation performance in HEAD, non-assert builds, with and without --disable-float8-byval on a 64-bit machine. So this tests replacing a pass-by-val transition datatype with a pass-by-ref one without

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

2014-04-09 Thread Florian Pflug
On Apr9, 2014, at 20:20 , Tom Lane t...@sss.pgh.pa.us wrote: There was discussion upthread of providing two separate forward transition functions, but Florian argued that that would do nothing that you couldn't accomplish with a runtime check in the forward function. I think that's nonsense

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

2014-04-09 Thread Tom Lane
Florian Pflug f...@phlo.org writes: I was (and still am) not in favour of duplicating the whole quadruple of (state, initialvalue, transferfunction, finalfunction) because it seems excessive. In fact, I believed that doing this would probably be grounds for outright rejection of the patch, on

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

2014-04-09 Thread Florian Pflug
On Apr9, 2014, at 23:17 , Florian Pflug f...@phlo.org wrote: On Apr9, 2014, at 21:35 , Tom Lane t...@sss.pgh.pa.us wrote: A quick test says that avg(int4) is about five percent slower than sum(int4), so that's the kind of hit we'd be taking on non-windowed aggregations if we do it like this.

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

2014-04-09 Thread Dean Rasheed
On 9 April 2014 22:55, Tom Lane t...@sss.pgh.pa.us wrote: Florian Pflug f...@phlo.org writes: I was (and still am) not in favour of duplicating the whole quadruple of (state, initialvalue, transferfunction, finalfunction) because it seems excessive. In fact, I believed that doing this would

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

2014-04-09 Thread Dean Rasheed
On 10 April 2014 01:13, Florian Pflug f...@phlo.org wrote: Also, the *only* reason that SUM(int2|int4) cannot use int8 as it's transition type is that it needs to return NULL, not 0, if zero rows were aggregates. It might seems that it could just use int8 as state with initial value NULL, but

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

2014-04-08 Thread David Rowley
On Wed, Apr 9, 2014 at 8:48 AM, Florian Pflug f...@phlo.org wrote: As explain above, invtrans_bool is a bit problematic, since it carries a real risk of performance regressions. It's included for completeness' sake, and should probably not be committed at this time. Did you mean to write

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

2014-04-08 Thread Florian Pflug
On Apr9, 2014, at 02:55 , David Rowley dgrowle...@gmail.com wrote: On Wed, Apr 9, 2014 at 8:48 AM, Florian Pflug f...@phlo.org wrote: As explain above, invtrans_bool is a bit problematic, since it carries a real risk of performance regressions. It's included for completeness' sake, and

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

2014-04-07 Thread Florian Pflug
On Apr5, 2014, at 09:55 , Dean Rasheed dean.a.rash...@gmail.com wrote: On 5 April 2014 08:38, Dean Rasheed dean.a.rash...@gmail.com wrote: [snip] releasing it in this state feels a little half-baked to me. I regret writing that almost as soon as I sent it. The last of those queries is now

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

2014-04-07 Thread Dean Rasheed
On 7 April 2014 14:09, Florian Pflug f...@phlo.org wrote: On Apr5, 2014, at 09:55 , Dean Rasheed dean.a.rash...@gmail.com wrote: On 5 April 2014 08:38, Dean Rasheed dean.a.rash...@gmail.com wrote: [snip] releasing it in this state feels a little half-baked to me. I regret writing that

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

2014-04-05 Thread Dean Rasheed
On 4 April 2014 11:56, Florian Pflug f...@phlo.org wrote: On 04.04.2014, at 09:40, Dean Rasheed dean.a.rash...@gmail.com wrote: I'm not sure how much additional work is required to sort this out, but to me it looks more realistic to target 9.5 than 9.4, so at this point I tend to think that

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

2014-04-05 Thread Dean Rasheed
On 5 April 2014 08:38, Dean Rasheed dean.a.rash...@gmail.com wrote: [snip] releasing it in this state feels a little half-baked to me. I regret writing that almost as soon as I sent it. The last of those queries is now over 10 times faster than HEAD, which is certainly worthwhile. What bugs me

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

2014-04-04 Thread Dean Rasheed
On 1 April 2014 20:58, Florian Pflug f...@phlo.org wrote: On Apr1, 2014, at 10:08 , Dean Rasheed dean.a.rash...@gmail.com wrote: On 31 March 2014 01:58, Florian Pflug f...@phlo.org wrote: Attached are updated patches that include the EXPLAIN changes mentioned above and updated docs. These

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

2014-04-04 Thread Florian Pflug
), which seem reasonable. But then I started testing performance, and I found cases where the improvement is not nearly what I expected. The example cited at the start of this thread is indeed orders of magnitude faster than HEAD: SELECT SUM(n::int) OVER (ROWS BETWEEN CURRENT ROW AND

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

2014-04-04 Thread Florian Pflug
On 04.04.2014, at 09:40, Dean Rasheed dean.a.rash...@gmail.com wrote: I'm not sure how much additional work is required to sort this out, but to me it looks more realistic to target 9.5 than 9.4, so at this point I tend to think that the patch ought to be marked as returned with feedback.

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

2014-04-04 Thread Andres Freund
On 2014-04-04 12:56:55 +0200, Florian Pflug wrote: On 04.04.2014, at 09:40, Dean Rasheed dean.a.rash...@gmail.com wrote: I'm not sure how much additional work is required to sort this out, but to me it looks more realistic to target 9.5 than 9.4, so at this point I tend to think that

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

2014-04-01 Thread Dean Rasheed
On 31 March 2014 01:58, Florian Pflug f...@phlo.org wrote: Attached are updated patches that include the EXPLAIN changes mentioned above and updated docs. These patches need re-basing --- they no longer apply to HEAD. Regards, Dean -- Sent via pgsql-hackers mailing list

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

2014-03-28 Thread Dean Rasheed
On 27 March 2014 21:01, Florian Pflug f...@phlo.org wrote: First, sorry guys for letting this slide - I was overwhelmed by other works, and this kind of slipped my mind :-( On Mar27, 2014, at 09:04 , Dean Rasheed dean.a.rash...@gmail.com wrote: On 26 March 2014 19:43, David Rowley

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

2014-03-27 Thread Dean Rasheed
On 26 March 2014 19:43, David Rowley dgrowle...@gmail.com wrote: On Thu, Mar 27, 2014 at 7:33 AM, Tom Lane t...@sss.pgh.pa.us wrote: David Rowley dgrowle...@gmail.com writes: I've attached an updated invtrans_strictstrict_base patch which has the feature removed. What is the state of play

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

2014-03-27 Thread Florian Pflug
First, sorry guys for letting this slide - I was overwhelmed by other works, and this kind of slipped my mind :-( On Mar27, 2014, at 09:04 , Dean Rasheed dean.a.rash...@gmail.com wrote: On 26 March 2014 19:43, David Rowley dgrowle...@gmail.com wrote: On Thu, Mar 27, 2014 at 7:33 AM, Tom Lane

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

2014-03-26 Thread Tom Lane
David Rowley dgrowle...@gmail.com writes: I've attached an updated invtrans_strictstrict_base patch which has the feature removed. What is the state of play on this patch? Is the latest version what's in http://www.postgresql.org/message-id/64f96fd9-64d1-40b9-8861-e61820292...@phlo.org plus

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

2014-03-26 Thread David Rowley
On Thu, Mar 27, 2014 at 7:33 AM, Tom Lane t...@sss.pgh.pa.us wrote: David Rowley dgrowle...@gmail.com writes: I've attached an updated invtrans_strictstrict_base patch which has the feature removed. What is the state of play on this patch? Is the latest version what's in

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

2014-03-07 Thread Tom Lane
Florian Pflug f...@phlo.org writes: On Mar5, 2014, at 18:37 , Tom Lane t...@sss.pgh.pa.us wrote: My advice is to lose the EXPLAIN output entirely. If the authors of the patch can't agree on what it means, what hope have everyday users got of making sense of it? The question isn't what the

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

2014-03-07 Thread Florian Pflug
On Mar5, 2014, at 23:49 , Tom Lane t...@sss.pgh.pa.us wrote: Florian Pflug f...@phlo.org writes: On Mar5, 2014, at 18:37 , Tom Lane t...@sss.pgh.pa.us wrote: My advice is to lose the EXPLAIN output entirely. If the authors of the patch can't agree on what it means, what hope have everyday

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

2014-03-07 Thread Florian Pflug
On Mar5, 2014, at 23:49 , Tom Lane t...@sss.pgh.pa.us wrote: Florian Pflug f...@phlo.org writes: On Mar5, 2014, at 18:37 , Tom Lane t...@sss.pgh.pa.us wrote: My advice is to lose the EXPLAIN output entirely. If the authors of the patch can't agree on what it means, what hope have everyday

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

2014-03-06 Thread Greg Stark
On Wed, Mar 5, 2014 at 10:49 PM, Tom Lane t...@sss.pgh.pa.us wrote: [ shrug... ] They can see whether the Window plan node is where the time is going. It's not apparent to me that the extra numbers you propose to report will edify anybody. Perhaps just saying Incremental Window Function

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

2014-03-05 Thread Florian Pflug
On Mar4, 2014, at 21:09 , Dean Rasheed dean.a.rash...@gmail.com wrote: On 3 March 2014 23:00, Florian Pflug f...@phlo.org wrote: * In show_windowagg_info(), this calculation looks suspicious to me: double tperrow = winaggstate-aggfwdtrans / (inst-nloops * inst-ntuples); If

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

2014-03-05 Thread Dean Rasheed
On 5 March 2014 14:35, Florian Pflug f...@phlo.org wrote: On Mar4, 2014, at 21:09 , Dean Rasheed dean.a.rash...@gmail.com wrote: On 3 March 2014 23:00, Florian Pflug f...@phlo.org wrote: * In show_windowagg_info(), this calculation looks suspicious to me: double tperrow =

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

2014-03-05 Thread Tom Lane
Dean Rasheed dean.a.rash...@gmail.com writes: I think we really need a larger consensus on this though, so I'd be interested to hear what others think. My advice is to lose the EXPLAIN output entirely. If the authors of the patch can't agree on what it means, what hope have everyday users got

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

2014-03-05 Thread Vladimir Sitnikov
Tom, I did not follow the thread very close, so I need to look what the ambiguity is there, however I would love to see window rescans in explain analyze. I have great experience in tuning Oracle queries. There are features in PostgreSQL's explain analyze that I miss badly in Oracle: 'rows

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

2014-03-05 Thread Florian Pflug
On Mar5, 2014, at 18:37 , Tom Lane t...@sss.pgh.pa.us wrote: Dean Rasheed dean.a.rash...@gmail.com writes: I think we really need a larger consensus on this though, so I'd be interested to hear what others think. My advice is to lose the EXPLAIN output entirely. If the authors of the patch

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

2014-03-05 Thread Florian Pflug
On Mar5, 2014, at 18:27 , Dean Rasheed dean.a.rash...@gmail.com wrote: On 5 March 2014 14:35, Florian Pflug f...@phlo.org wrote: When I added the EXPLAIN stuff, I initially simply reported the number of times nodeWindowAgg has to restart the aggregation. The problem with that approach is that

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

2014-03-04 Thread Dean Rasheed
On 3 March 2014 23:00, Florian Pflug f...@phlo.org wrote: * In show_windowagg_info(), this calculation looks suspicious to me: double tperrow = winaggstate-aggfwdtrans / (inst-nloops * inst-ntuples); If the node is executed multiple times, aggfwdtrans will be reset in each

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

2014-03-02 Thread Dean Rasheed
On 25 February 2014 12:33, Florian Pflug f...@phlo.org wrote: On Feb24, 2014, at 17:50 , Dean Rasheed dean.a.rash...@gmail.com wrote: On 20 February 2014 01:48, Florian Pflug f...@phlo.org wrote: On Jan29, 2014, at 13:45 , Florian Pflug f...@phlo.org wrote: In fact, I'm currently leaning

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

2014-02-25 Thread Florian Pflug
On Feb24, 2014, at 17:50 , Dean Rasheed dean.a.rash...@gmail.com wrote: On 20 February 2014 01:48, Florian Pflug f...@phlo.org wrote: On Jan29, 2014, at 13:45 , Florian Pflug f...@phlo.org wrote: In fact, I'm currently leaning towards just forbidding non-strict forward transition function

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

2014-02-24 Thread Dean Rasheed
On 20 February 2014 01:48, Florian Pflug f...@phlo.org wrote: On Jan29, 2014, at 13:45 , Florian Pflug f...@phlo.org wrote: In fact, I'm currently leaning towards just forbidding non-strict forward transition function with strict inverses, and adding non-NULL counters to the aggregates that

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

2014-01-29 Thread Dean Rasheed
On 28 January 2014 20:16, Florian Pflug f...@phlo.org wrote: On Jan27, 2014, at 23:28 , Dean Rasheed dean.a.rash...@gmail.com wrote: strict transfn vs non-strict inv_transfn This case is explicitly forbidden, both in CREATE AGGREGATE and in the

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

2014-01-29 Thread Florian Pflug
On Jan29, 2014, at 09:59 , Dean Rasheed dean.a.rash...@gmail.com wrote: On 28 January 2014 20:16, Florian Pflug f...@phlo.org wrote: On Jan27, 2014, at 23:28 , Dean Rasheed dean.a.rash...@gmail.com wrote: This case is explicitly forbidden, both in CREATE AGGREGATE and in the executor. To me,

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

2014-01-28 Thread Florian Pflug
On Jan27, 2014, at 23:28 , Dean Rasheed dean.a.rash...@gmail.com wrote: strict transfn vs non-strict inv_transfn This case is explicitly forbidden, both in CREATE AGGREGATE and in the executor. To me, that seems overly restrictive --- if transfn is

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

2014-01-27 Thread Dean Rasheed
On 25 January 2014 02:21, Florian Pflug f...@phlo.org wrote: I've now split this up into invtrans_base: Basic machinery plus tests with SQL-language aggregates invtrans_arith: COUNT(),SUM(),AVG(),STDDEV() and the like invtrans_minmax: MIN(),MAX(),BOOL_AND(),BOOL_OR() invtrans_collecting:

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

2014-01-26 Thread Florian Pflug
On Jan26, 2014, at 00:24 , David Rowley dgrowle...@gmail.com wrote: On Sat, Jan 25, 2014 at 3:21 PM, Florian Pflug f...@phlo.org wrote: On Jan24, 2014, at 08:47 , Dean Rasheed dean.a.rash...@gmail.com wrote: The invtrans_minmax patch doesn't contain any patches yet - David, could you provide

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

2014-01-25 Thread David Rowley
On Thu, Jan 23, 2014 at 1:57 PM, Florian Pflug f...@phlo.org wrote: On Jan23, 2014, at 01:17 , David Rowley dgrowle...@gmail.com wrote: On Wed, Jan 22, 2014 at 12:46 AM, Florian Pflug f...@phlo.org wrote: If you want to play with this, I think the first step has to be to find a set of

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

2014-01-25 Thread Tom Lane
David Rowley dgrowle...@gmail.com writes: My point was more that since sum(float) can give different results if it used an index scan rather than a seq scan, trying to get the inverse transition to match something that gives varying results sounds like a tricky task to take on. This is just a

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

2014-01-25 Thread Florian Pflug
On Jan25, 2014, at 09:50 , David Rowley dgrowle...@gmail.com wrote: On Thu, Jan 23, 2014 at 1:57 PM, Florian Pflug f...@phlo.org wrote: On Jan23, 2014, at 01:17 , David Rowley dgrowle...@gmail.com wrote: On Wed, Jan 22, 2014 at 12:46 AM, Florian Pflug f...@phlo.org wrote: If you want to play

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

2014-01-25 Thread David Rowley
On Sat, Jan 25, 2014 at 3:21 PM, Florian Pflug f...@phlo.org wrote: On Jan24, 2014, at 08:47 , Dean Rasheed dean.a.rash...@gmail.com wrote: The invtrans_minmax patch doesn't contain any patches yet - David, could you provide some for these functions, and also for bool_and and bool_or? We

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

2014-01-24 Thread Florian Pflug
On Jan24, 2014, at 08:47 , Dean Rasheed dean.a.rash...@gmail.com wrote: I think it should probably be broken up. It might be overly ambitious to try to get all of this committed during this commitfest, and in any case, I suspect that the committer would probably choose to commit it in stages.

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

2014-01-23 Thread Dean Rasheed
On 22 January 2014 09:54, David Rowley dgrowle...@gmail.com wrote: I've performed some more benchmarks on this patch tonight. The results and full recreation scripts are attached along with the patch it was tested against. I noticed that the rate of changes to this patch has dropped off,

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

2014-01-22 Thread David Rowley
On Tue, Jan 21, 2014 at 3:20 AM, Florian Pflug f...@phlo.org wrote: On Jan20, 2014, at 08:42 , David Rowley dgrowle...@gmail.com wrote: On Mon, Jan 20, 2014 at 2:45 PM, Florian Pflug f...@phlo.org wrote: * I've also renamed INVFUNC to INVSFUNC. That's a pretty invasive change, and it's

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

2014-01-22 Thread David Rowley
On Wed, Jan 22, 2014 at 12:46 AM, Florian Pflug f...@phlo.org wrote: On Jan21, 2014, at 10:53 , David Rowley dgrowle...@gmail.com wrote: It came to me that it might be possible to implement inverse transitions for floating point aggregates by just detecting if precision has been lost

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

2014-01-22 Thread Florian Pflug
On Jan23, 2014, at 01:17 , David Rowley dgrowle...@gmail.com wrote: On Wed, Jan 22, 2014 at 12:46 AM, Florian Pflug f...@phlo.org wrote: If you want to play with this, I think the first step has to be to find a set of guarantees that SUM(float) is supposed to meet. Currently, SUM(float)

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

2014-01-22 Thread Florian Pflug
On Jan23, 2014, at 01:07 , David Rowley dgrowle...@gmail.com wrote: On Tue, Jan 21, 2014 at 3:20 AM, Florian Pflug f...@phlo.org wrote: On Jan20, 2014, at 08:42 , David Rowley dgrowle...@gmail.com wrote: On Mon, Jan 20, 2014 at 2:45 PM, Florian Pflug f...@phlo.org wrote: * I've also renamed

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

2014-01-21 Thread David Rowley
On Sun, Dec 15, 2013 at 2:00 PM, Tom Lane t...@sss.pgh.pa.us wrote: Greg Stark st...@mit.edu writes: On 14 Dec 2013 15:40, Tom Lane t...@sss.pgh.pa.us wrote: I think you *can't* cover them for the float types; roundoff error would mean you don't get the same answers as before. I was

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

2014-01-21 Thread Florian Pflug
On Jan21, 2014, at 10:53 , David Rowley dgrowle...@gmail.com wrote: It came to me that it might be possible to implement inverse transitions for floating point aggregates by just detecting if precision has been lost during forward transitions. I've written the test to do this as: IF

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

2014-01-21 Thread Florian Pflug
On Jan20, 2014, at 15:20 , Florian Pflug f...@phlo.org wrote: * In CREATE AGGREGATE, we should state the precise axioms we assume about forward and inverse transition functions. The last time I read the text there, it was a bit ambiguous about whether inverse transition functions assume

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

2014-01-20 Thread David Rowley
On Mon, Jan 20, 2014 at 8:42 PM, David Rowley dgrowle...@gmail.com wrote: On Mon, Jan 20, 2014 at 2:45 PM, Florian Pflug f...@phlo.org wrote: * EXPLAIN VERBOSE ANALYZE now shows the max. number of forward aggregate transitions per row and aggregate. It's a bit imprecise, because it doesn't

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

2014-01-20 Thread Florian Pflug
On Jan19, 2014, at 20:00 , David Rowley dgrowle...@gmail.com wrote: I've applied that patch again and put in the sort operators. I've push a new version to https://github.com/fgp/postgres/tree/invtrans which includes * A bunch of missing declaration for *_inv functions * An assert that the

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

2014-01-20 Thread David Rowley
On Mon, Jan 20, 2014 at 2:45 PM, Florian Pflug f...@phlo.org wrote: On Jan19, 2014, at 20:00 , David Rowley dgrowle...@gmail.com wrote: I've applied that patch again and put in the sort operators. I've push a new version to https://github.com/fgp/postgres/tree/invtrans which includes * A

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

2014-01-20 Thread Florian Pflug
On Jan20, 2014, at 08:42 , David Rowley dgrowle...@gmail.com wrote: On Mon, Jan 20, 2014 at 2:45 PM, Florian Pflug f...@phlo.org wrote: * An assert that the frame end doesn't move backwards - I realized that it is after all easy to do that, if it's done after the loop which adds the new

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

2014-01-19 Thread David Rowley
On Sun, Jan 19, 2014 at 5:27 PM, David Rowley dgrowle...@gmail.com wrote: It's probably far more worth it for the bool and/or aggregates. We could just keep track of the values aggregated and the count of values as true and return true if those are the same in the case of AND, then check

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

2014-01-19 Thread David Rowley
On Sat, Jan 18, 2014 at 11:34 AM, David Rowley dgrowle...@gmail.com wrote: The latest version of the patch is attached. I've attached an updated version of the patch. I'm now using github to track the changes on the patch, so I've included the commit sha in the file name of the latest commit

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

2014-01-19 Thread Florian Pflug
On Jan19, 2014, at 05:27 , David Rowley dgrowle...@gmail.com wrote: I just finished implementing the inverse transition functions for bool_and and bool_or, these aggregates had a sort operator which I assume would have allowed an index scan to be performed, but since I had to change the first

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

2014-01-19 Thread David Rowley
On Mon, Jan 20, 2014 at 5:53 AM, Florian Pflug f...@phlo.org wrote: On Jan19, 2014, at 05:27 , David Rowley dgrowle...@gmail.com wrote: I just finished implementing the inverse transition functions for bool_and and bool_or, these aggregates had a sort operator which I assume would have

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

2014-01-19 Thread Florian Pflug
On Jan19, 2014, at 20:00 , David Rowley dgrowle...@gmail.com wrote: I've applied that patch again and put in the sort operators. I've push a new version to https://github.com/fgp/postgres/tree/invtrans This branch includes the following changes * A bunch of missing declaration for *_inv

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

2014-01-18 Thread Florian Pflug
On Jan18, 2014, at 06:15 , David Rowley dgrowle...@gmail.com wrote: On Sat, Jan 18, 2014 at 2:20 PM, Florian Pflug f...@phlo.org wrote: On Jan17, 2014, at 23:34 , David Rowley dgrowle...@gmail.com wrote: The test turned out to become: if (state-expectedScale == -1)

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

2014-01-18 Thread David Rowley
On Sun, Jan 19, 2014 at 3:22 AM, Florian Pflug f...@phlo.org wrote: BTW, this made me realize that MIN and MAX currently have the same issue - they'll rescan if the inputs are all equal. We could avoid that by doing what you did with dscale - track the number of times we've seen the maximum.

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

2014-01-17 Thread David Rowley
On Fri, Jan 17, 2014 at 3:05 PM, Florian Pflug f...@phlo.org wrote: I had some more fun with this, the result is v2.5 of the patch (attached). Changes are explained below. Looks like you've been busy on this! Thank you for implementing all the changes you talked about. I've now started

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

2014-01-17 Thread Florian Pflug
On Jan17, 2014, at 15:14 , David Rowley dgrowle...@gmail.com wrote: If you make any more changes would it be possible for you to base them on the attached patch instead of the last one you sent? Sure! The only reason I didn't rebase my last patch onto yours was that having the numeric stuff

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

2014-01-17 Thread David Rowley
On Fri, Jan 17, 2014 at 3:05 PM, Florian Pflug f...@phlo.org wrote: I've now shuffled things around so that we can use inverse transition functions even if only some aggregates provide them, and to allow inverse transition functions to force a restart by returning NULL. The rules regarding

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

2014-01-17 Thread Florian Pflug
On Jan17, 2014, at 20:34 , David Rowley dgrowle...@gmail.com wrote: On Fri, Jan 17, 2014 at 3:05 PM, Florian Pflug f...@phlo.org wrote: I've now shuffled things around so that we can use inverse transition functions even if only some aggregates provide them, and to allow inverse transition

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

2014-01-17 Thread David Rowley
On Sat, Jan 18, 2014 at 10:17 AM, Florian Pflug f...@phlo.org wrote: On Jan17, 2014, at 20:34 , David Rowley dgrowle...@gmail.com wrote: On Fri, Jan 17, 2014 at 3:05 PM, Florian Pflug f...@phlo.org wrote: I've now shuffled things around so that we can use inverse transition functions

  1   2   3   >