Re: onlyvalue aggregate (was: First Aggregate Funtion?)

From: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
To: Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>
Cc: Marko Tiikkaja <marko(at)joh(dot)to>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: onlyvalue aggregate (was: First Aggregate Funtion?)
Date: 2015-12-24 02:49:05
Message-ID: CAB7nPqQ=xma_0vuGK5rBP0YmSGCBVK__vqQ4ZMJJdUA=q+gVYA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Mon, Nov 23, 2015 at 6:29 PM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> On 21 November 2015 at 03:54, Marko Tiikkaja <marko(at)joh(dot)to> wrote:
>> Here's v2 of the patch. How's this look?
>>
>
> Here are some initial review comments:
>
> * My first thought on reading this patch is that it is somewhat
> under-commented. For example, I would expect at least a block comment
> at the top of the new code added by this patch. Also the fields of the
> new structure could use some comments -- it might be obvious what
> datum and isnull are for, but fcinfo is less obvious until you read
> the code that uses it. Likewise the transfn is quite long, with almost
> no explanatory comments.
>
> * There's a clear bug in the null handling in the second branch of the
> transfn -- when the new value is null and the previous value wasn't.
> For example:
>
> SELECT single_value(x) FROM (VALUES ('x'), (null)) t(x);
> server closed the connection unexpectedly
>
> * In the finalfn, I think calling AggCheckCallContext() should be the
> first thing it does. Compare for example array_agg_array_finalfn().
>
> * There's another less obvious bug in the way these functions handle
> complex types. For example:
>
> SELECT single_value(t) FROM (VALUES (1,'One'), (1,'One')) t(x,y);
> ERROR: cache lookup failed for type 2139062143
>
> You might want to look at how array_agg() handles that. Also the code
> behind array_position() has some elements in common with this patch.
>
> * Consider collation handling, as illustrated by array_position().
>
> So I'm afraid there's still some work to do, but there are plenty of
> examples in existing code to borrow from.

There is a review, but no input from the author for more than 1 month,
hence patch has been marked as "returned with feedback". Feel free to
move it to next CF if you want to post a new version.
--
Michael

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2015-12-24 02:50:47 Re: Speed up Clog Access by increasing CLOG buffers
Previous Message Amit Langote 2015-12-24 02:46:09 Re: A Typo in regress/sql/privileges.sql