Re: Improving avg performance for numeric

Hi,

I've reviewed the v6 of the "numeric optimize" patch
(http://www.postgresql.org/message-id/CAFj8pRDQhG7Pqmf8XqXY0PnHfakkPQLPHnoRLJ_=EKFSbOAWeA@mail.gmail.com),
as Pavel did some hacking on the patch and asked me to do the review.

The patch seems fine to me, the following comments are mostly nitpicking:

1) Applies cleanly to the HEAD (although only by "patch" and not "git
apply").

2) I think we should use "estimate" instead of "approximation" in the
docs, it seems more correct / natural to me (but maybe I'm wrong on this
one).

3) state_data_size does not make much sense to me - it should be
state_size. This probably comes from the state_data_type, but that's
('state' + 'data type') and by replacing the second part with 'size'
you'll get state_size.

4) currently the state size may be specified for all data types, no
matter if their size is fixed or variable. Wouldn't it be safer to allow
this option only for variable-length data types? Right now you can
specify stype=int and sspace=200 which does not make much sense to me.
We can always relax the restrictions if needed, the opposite is much
more difficult.

5) in numeric.c, there are no comments for
- fields of the NumericAggState (some are obvious, some are not)
- multiple functions are missing the initial comments (e.g.
numeric_accum, do_numeric_accum)

6) I think the "first" variable in do_numeric_accum is rather useless,
it's trivial to remove it - just use the state->first instead and move
the assignment to the proper branch (ok, this is nitpicking).

7) I think the error message in makeNumericAggState should be something
like "This must not be called in non-aggregate context!" or something
like that.

8) The records in pg_aggregate.c are using either 0 (for fixed-length)
or 128. This seems slightly excessive to me. What is the reasoning
behind this? Is that because of the two NumericVar fields?

regards
Tomas

Hello

2013/9/22 Tomas Vondra <tv(at)fuzzy(dot)cz>

> Hi,
>
> I've reviewed the v6 of the "numeric optimize" patch
> (http://www.postgresql.org/**message-id/**CAFj8pRDQhG7Pqmf8XqXY0PnHfakkP**
> QLPHnoRLJ_=EKFSbOAWeA(at)mail(dot)**gmail(dot)com<http://www.postgresql.org/message-id/CAFj8pRDQhG7Pqmf8XqXY0PnHfakkPQLPHnoRLJ_=EKFSbOAWeA@mail.gmail.com>
> ),
> as Pavel did some hacking on the patch and asked me to do the review.
>
> The patch seems fine to me, the following comments are mostly nitpicking:
>
> 1) Applies cleanly to the HEAD (although only by "patch" and not "git
> apply").
>
> 2) I think we should use "estimate" instead of "approximation" in the
> docs, it seems more correct / natural to me (but maybe I'm wrong on this
> one).
>
> 3) state_data_size does not make much sense to me - it should be
> state_size. This probably comes from the state_data_type, but that's
> ('state' + 'data type') and by replacing the second part with 'size'
> you'll get state_size.
>

This name is consistent with previous field state_data_type - I expected so
this mean 'state data' + 'type'. I am not native speaker, so my position is
not strong, but in this moment I am thinking so state_data_size has a
sense. In this case both variant has sense - 'state data' + type or
'state' + 'data type'.

>
> 4) currently the state size may be specified for all data types, no
> matter if their size is fixed or variable. Wouldn't it be safer to allow
> this option only for variable-length data types? Right now you can
> specify stype=int and sspace=200 which does not make much sense to me.
> We can always relax the restrictions if needed, the opposite is much
> more difficult.
>
>
good idea

> 5) in numeric.c, there are no comments for
> - fields of the NumericAggState (some are obvious, some are not)
> - multiple functions are missing the initial comments (e.g.
> numeric_accum, do_numeric_accum)
>

ok

>
> 6) I think the "first" variable in do_numeric_accum is rather useless,
> it's trivial to remove it - just use the state->first instead and move
> the assignment to the proper branch (ok, this is nitpicking).
>

ok

>
> 7) I think the error message in makeNumericAggState should be something
> like "This must not be called in non-aggregate context!" or something like
> that.
>

I used a "a numeric aggregate function called in non-aggregate context" -
it is similar to other related messages

>
> 8) The records in pg_aggregate.c are using either 0 (for fixed-length) or
> 128. This seems slightly excessive to me. What is the reasoning behind
> this? Is that because of the two NumericVar fields?
>

NumericAggState has 96 bytes - but you have to add a space for digits of
included numeric values (inclued in NumericVar) -- so it is others 16 + 16
= 128. I am not able to specify how much digits will be used exactly - 16
bytes is just good enough estimation - it is not used for memory
allocation, it is used for some planner magic.

>
> regards
> Tomas
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/**mailpref/pgsql-hackers<http://www.postgresql.org/mailpref/pgsql-hackers>
>

On 23 Září 2013, 18:18, Pavel Stehule wrote:
> Hello
>
>
> 2013/9/22 Tomas Vondra <tv(at)fuzzy(dot)cz>
>
>> Hi,
>>
>> I've reviewed the v6 of the "numeric optimize" patch
>> (http://www.postgresql.org/**message-id/**CAFj8pRDQhG7Pqmf8XqXY0PnHfakkP**
>> QLPHnoRLJ_=EKFSbOAWeA(at)mail(dot)**gmail(dot)com<http://www.postgresql.org/message-id/CAFj8pRDQhG7Pqmf8XqXY0PnHfakkPQLPHnoRLJ_=EKFSbOAWeA@mail.gmail.com>
>> ),
>> as Pavel did some hacking on the patch and asked me to do the review.
>>
>> The patch seems fine to me, the following comments are mostly
>> nitpicking:
>>
>> 1) Applies cleanly to the HEAD (although only by "patch" and not "git
>> apply").
>>
>> 2) I think we should use "estimate" instead of "approximation" in the
>> docs, it seems more correct / natural to me (but maybe I'm wrong on this
>> one).
>>
>> 3) state_data_size does not make much sense to me - it should be
>> state_size. This probably comes from the state_data_type, but that's
>> ('state' + 'data type') and by replacing the second part with 'size'
>> you'll get state_size.
>>
>
> This name is consistent with previous field state_data_type - I expected
> so
> this mean 'state data' + 'type'. I am not native speaker, so my position
> is
> not strong, but in this moment I am thinking so state_data_size has a
> sense. In this case both variant has sense - 'state data' + type or
> 'state' + 'data type'.

OK, let's leave this up to a native speaker.

>>
>> 8) The records in pg_aggregate.c are using either 0 (for fixed-length)
>> or
>> 128. This seems slightly excessive to me. What is the reasoning behind
>> this? Is that because of the two NumericVar fields?
>>
>
> NumericAggState has 96 bytes - but you have to add a space for digits of
> included numeric values (inclued in NumericVar) -- so it is others 16 + 16
> = 128. I am not able to specify how much digits will be used exactly - 16
> bytes is just good enough estimation - it is not used for memory
> allocation, it is used for some planner magic.

OK, makes sense.

I've made some basic tests on a 40M table with random numerics, for
example this query:

select avg(val), sum(val), var_pop(val) from numeric_test ;

takes ~57 seconds on current master, but only ~26 seconds with the v7
patch. Granted, in practice the improvements won't be as good because of
I/O costs etc., but it's a nice gain.

Seems "ready for commiter" to me. I'll wait a few days for others to
comment, and then I'll update the commitfest page.

regards
Tomas

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?

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

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
>

On 24.9.2013 17:57, Pavel Stehule wrote:
>
>
>
> 2013/9/24 Robert Haas <robertmhaas(at)gmail(dot)com
> <mailto:robertmhaas(at)gmail(dot)com>>
>
> On Mon, Sep 23, 2013 at 4:15 PM, Tomas Vondra <tv(at)fuzzy(dot)cz
> <mailto: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.

I share this oppinion. I was not able to come up with a single use case
benefiting from allowing this for types other than internal. Seems like
a footgun to me, with no potential benefit.

So +1 to keeping the patch 'type internal only' from me.

With the formatting issues now fixed, I believe the patch is ready for
committer (and someone already switched it to that state).

Tomas

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> [ numeric-optimize-v8.patch ]

I've committed this with some adjustments. Aside from cosmetic
changes and documentation/comment improvements:

* I don't agree at all with the claim that aggtransspace is only useful
for INTERNAL transition data. There are lots of cases with regular
SQL types where the planner doesn't have a good idea of how large the
transition value will be, and with the current code it tends to guess
on the small side (frequently 32 bytes :-(). It seems to me to be
useful to give users a knob to twiddle there. Consider for example
an aggregate that uses an integer array as transition state; the author
of the aggregate might know that there are usually hundreds of entries
in the array, and wish to dial up aggtransspace to prevent the planner
from optimistically choosing hash aggregation.

As committed, I just took out the restriction in CREATE AGGREGATE
altogether; it will let you set SSPACE for any transition datatype.
The planner will ignore it for pass-by-value types, where the behavior
is known, but otherwise honor it. It's possible that we should put in
a restriction to INTERNAL plus pass-by-reference types, or even INTERNAL
plus pass-by-reference variable-length types, but I can't get excited
about that. The error message would be too confusing I think.

* There was a stray "else" added to clauses.c that disabled space
accounting for polymorphic aggregates.

* I simplified the summing code in do_numeric_accum; the copying and
reallocating it was doing was not only unnecessary but contained
unpleasant assumptions about what you can do with a NumericVar. This
probably makes the committed patch a bit faster than what was submitted,
but I didn't try to benchmark the change.

* I made sure all the functions accessed their input state pointer with
state = PG_ARGISNULL(0) ? NULL : (NumericAggState *) PG_GETARG_POINTER(0);
There were places that omitted the ARGISNULL test, and thus only happened
to work if the uninitialized Datum value they got was zero. While that
might chance to be true in the current state of the code, it's not an
assumption you're supposed to make.

* numeric sum/avg failed to check for NaN inputs.

* I fixed incorrect tests in the code added to pg_dump.

regards, tom lane