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

From: David Rowley <dgrowleyml(at)gmail(dot)com>
To: Florian Pflug <fgp(at)phlo(dot)org>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Kevin Grittner <kgrittn(at)ymail(dot)com>, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>, Josh Berkus <josh(at)agliodbs(dot)com>, Greg Stark <stark(at)mit(dot)edu>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [PATCH] Negative Transition Aggregate Functions (WIP)
Date: 2014-01-16 08:07:16
Message-ID: CAApHDvrG6_hmukTex5sYV9KxPQiqs=u8PaEgp_TotuMeo+69jw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Wed, Jan 15, 2014 at 5:39 AM, Florian Pflug <fgp(at)phlo(dot)org> wrote:

The notnullcount machinery seems to apply to both STRICT and non-STRICT
> transfer function pairs. Shouldn't that be constrained to STRICT transfer
> function pairs? For non-STRICT pairs, it's up to the transfer functions to
> deal with NULL inputs however they please, no?
>
>
The reason I had to track the notnullcount was because for non-strict was
because:

select sum(n) over (order by i rows between current row and unbounded
following) from (values(1,1),(2,NULL)) v(i,n);

would otherwise return

1
0

sum is not strict, so I guess we need to track notnullcount for non-strict
functions.

See the code around if (peraggstate->notnullcount == 0)
in retreat_windowaggregate().

> The logic around movedaggbase in eval_windowaggregates() seems a bit
> convoluted. Couldn't the if be moved before the code that pulls
> aggregatedbase up to frameheadpos using the inverse aggregation function?
>
>
I had a look at this and even tried moving the code to before the inverse
transitions, but it looks like that would only work if I added more tests
to the if condition to ensure that we're not about to perform inverse
transitions. To me it just seemed more bullet proof the way it is rather
than duplicating logic and having to ensure that it stays duplicated. But
saying that I don't think I've fully got my head around why the original
code is valid in the first place. I would have imagined that it should
contain a check for FRAMEOPTION_START_UNBOUNDED_FOLLOWING, but if that
sounds like complete rubbish then I'll put it down to my head still being
fried from work.

> Also, could we easily check whether the frames corresponding to the
> individual rows are all either identical or disjoint, and don't use the
> inverse transfer function then? Currently, for a frame which contains
> either just the current row, or all the current row's peers, I think we'd
> use the inverse transfer function to fully un-add the old frame, and then
> add back the new frame.
>
>
I didn't know there was a situation where this could happen. Could you give
me an example query and I'll run it through the debugger to have a look at
what's going on. But sure, if this is possible and I understand what you
mean then I guess it would be a good optimisation to detect this and throw
away the previous results and start fresh.

Thanks for all of your reviewing on this so far.

Regards

David Rowley

> best regards,
> Florian Pflug
>
>

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message knizhnik 2014-01-16 08:30:55 Re: [Lsf-pc] Linux kernel impact on PostgreSQL performance
Previous Message David Rowley 2014-01-16 07:27:17 Re: [PATCH] Negative Transition Aggregate Functions (WIP)