Re: Improving avg performance for numeric

On Mon, Sep 23, 2013 at 4:15 PM, Tomas Vondra <tv@fuzzy.cz> wrote:
> Seems "ready for commiter" to me. I'll wait a few days for others to
> comment, and then I'll update the commitfest page.

Some thoughts:

The documentation doesn't reflect the restriction to type internal.
On a related note, why restrict this to type internal?

Formatting fixes are needed:

+               if (aggtransspace > 0)
+               {
+                       costs->transitionSpace += aggtransspace;
+               }

Project style is not to use curly-braces for single statements.  Also,
the changes to numeric.c add blank lines before and after function
header comments, which is not the usual style.

!       if (state == NULL)
!               PG_RETURN_NULL();
!       else
!               PG_RETURN_POINTER(state);

I think this should just say PG_RETURN_POINTER(state).  PG_RETURN_NULL
is for returning an SQL NULL, not (void *) 0.  Is there some reason
why we need an SQL NULL here, rather than a NULL pointer?

On the whole this looks fairly solid on a first read-through.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company