Re: Improving avg performance for numeric

2013/9/24 Robert Haas <robertmhaas(at)gmail(dot)com>

> On Mon, Sep 23, 2013 at 4:15 PM, Tomas Vondra <tv(at)fuzzy(dot)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?
>

Now, for almost all types Postgres well estimate size of state value. Only
arrays with unknown size can be a different. When we enable this value for
all types, then users can specify some bad values for scalar buildin types.
Next argument is simply and bad - I don't see a good use case for
customization this value for other than types than internal type.

I have no strong position here - prefer joining with internal type due
little bit higher robustness.

>
> 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?
>

fixed

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

Thank you

Pavel

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