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

On 27 March 2014 21:01, Florian Pflug <fgp(at)phlo(dot)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(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>> On 26 March 2014 19:43, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
>>> On Thu, Mar 27, 2014 at 7:33 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>>
>>>> David Rowley <dgrowleyml(at)gmail(dot)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-E6182029220B@phlo.org
>>>> plus this sub-patch? Is everybody reasonably happy with it? I don't
>>>> see it marked "ready for committer" in the CF app, but time is running
>>>> out.
>>>>
>>>
>>> As far as I know the only concern left was around the extra stats in the
>>> explain output, which I removed in the patch I attached in the previous
>>> email.
>>>
>>
>> Agreed. That was my last concern regarding the base patch, and I agree
>> that removing the new explain output is probably the best course of
>> action, given that we haven't reached consensus as to what the most
>> useful output would be.
>
> After re-reading the thread, I'd prefer to go with Dean's suggestion, i.e.
> simply reporting the total number of invocations of the forward transition
> functions, and the total number of invocations of the reverse transition
> function, over reporting nothing. The labels of the two counts would simply
> be "Forward Transitions" and "Reverse Transitions".
>

That should be "Inverse" not "Reverse" according to the terminology
agreed upthread.

Personally, I'm not a big fan of that terminology because "forward"
and "inverse" aren't natural antonyms. But actually I think that it's
"forward" that is the wrong word to use, because they actually both
move (different ends of) the frame forwards. The only alternatives I
can think of are "direct" and "inverse", which are natural antonyms,
but I don't want to hold up this patch bikeshedding over this. OTOH
this is not the first time on this thread that someone has slipped
into calling them "forward" and "reverse" transitions.

> But I don't want this issue to prevent us from getting this patch into 9.4,
> so if there are objections to this, I'll rip out the EXPLAIN stuff all
> together.
>
>>> The invtrans_strictstrict_base.patch in my previous email replaces the
>>> invtrans_strictstrict_base_038070.patch in that Florian sent here
>>> http://www.postgresql.org/message-id/64F96FD9-64D1-40B9-8861-E6182029220B@phlo.org
>>> all of the other patches are unchanged so it's save to use Florian's latest
>>> ones
>>>
>>> Perhaps Dean can confirm that there's nothing else outstanding?
>>>
>>
>> Florian mentioned upthread that the docs hadn't been updated to
>> reflect the latest changes, so I think they need a little attention.
>
> I'll see to updating the docs, and will post a final patch within the next
> few days.
>
> Dean, have you by chance looked at the other patches yet?
>

No, sorry. I too have been swamped by other work. I will try to look
at them over the next few days. I don't anticipate that they will be
as complex as the base patch, so I hope that this can be finished in
time for 9.4.

If there are any other reviewers with spare cycles, feel free to jump in too.

Regards,
Dean