From: | Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Tomas Vondra <tv(at)fuzzy(dot)cz>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Improving avg performance for numeric |
Date: | 2013-09-24 15:57:21 |
Message-ID: | CAFj8pRDMCsY+fa99xmw1mzhzUjj=i2AgyN9KerS=ZSYnZVpDHA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
>
Attachment | Content-Type | Size |
---|---|---|
numeric-optimize-v8.patch | application/octet-stream | 60.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Stephen Frost | 2013-09-24 16:00:35 | Re: record identical operator |
Previous Message | Steve Singer | 2013-09-24 15:51:02 | Re: logical changeset generation v6 |