SEO

On Fri, 2006-11-24 at 11:08 +1300, Mark Kirkwood wrote:
> - Modifies do_numeric_accum to have an extra bool parameter and does not
> calc sumX2 when it is false.

I think it would be clearer to reorganize this function slightly, and
have only a single branch on "useSumX2". On first glance it isn't
obviously that transdatums[2] is defined (but unchanged) when useSumX2
is false.

> Performance gain is approx 33%

Nice.

> (it is still slower than doing sum/count - possibly due to the
> construct/deconstruct overhead of the numeric transition array).

This would indeed be worth profiling. If it turns out that array
overhead is significant, I wonder if we could use a composite type for
the transition variable instead. That might also make it easier to
represent the "N" value as an int8 rather than a numeric.

-Neil



---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

Neil Conway wrote:

>
>> (it is still slower than doing sum/count - possibly due to the
>> construct/deconstruct overhead of the numeric transition array).
>
> This would indeed be worth profiling. If it turns out that array
> overhead is significant, I wonder if we could use a composite type for
> the transition variable instead. That might also make it easier to
> represent the "N" value as an int8 rather than a numeric.
>

I've profiled the 2nd patch using the setup indicated below. The first
64 lines of the flat graph are attached. The complete profile is here:

http://homepages.paradise.net.nz/mar...s-avg.gprof.gz

Setup:

avg=# \d avgtest
Table "public.avgtest"
Column | Type | Modifiers
--------+---------------+-----------
id | integer |
val0 | bigint |
val1 | numeric(12,2) |
val2 | numeric(10,0) |

avg=# analyze verbose avgtest;
INFO: analyzing "public.avgtest"
INFO: "avgtest": scanned 3000 of 87689 pages, containing 342138 live
rows and 0 dead rows; 3000 rows in sample, 10000580 estimated total rows
ANALYZE
Time: 252.033 ms
avg=# select avg(val2) from avgtest;
avg
---------------------
714285.214285800000
(1 row)

Time: 35196.028 ms
avg=# \q

regards

Mark

Flat profile:

Each sample counts as 0.01 seconds.
% cumulative self self total
time seconds seconds calls s/call s/call name
14.42 2.16 2.16 100002977 0.00 0.00 AllocSetAlloc
9.08 3.52 1.36 20000000 0.00 0.00 add_abs
5.54 4.35 0.83 10000000 0.00 0.00 slot_deform_tuple
5.41 5.16 0.81 60001673 0.00 0.00 AllocSetFree
4.34 5.81 0.65 10000000 0.00 0.00 construct_md_array
4.21 6.44 0.63 20000003 0.00 0.00 make_result
3.54 6.97 0.53 10000000 0.00 0.00 numeric_add
3.27 7.46 0.49 30000003 0.00 0.00 set_var_from_num
3.00 7.91 0.45 100002652 0.00 0.00 MemoryContextAlloc
2.74 8.32 0.41 10000001 0.00 0.00 heapgettup_pagemode
2.54 8.70 0.38 10000000 0.00 0.00 advance_transition_function
2.40 9.06 0.36 30000006 0.00 0.00 alloc_var
2.27 9.40 0.34 10000000 0.00 0.00 do_numeric_avg_accum
2.00 9.70 0.30 10000001 0.00 0.00 CopyArrayEls
2.00 10.00 0.30 10000000 0.00 0.00 numeric_inc
1.94 10.29 0.29 20000002 0.00 0.00 ArrayGetNItems
1.94 10.58 0.29 10000001 0.00 0.00 deconstruct_array
1.87 10.86 0.28 20000002 0.00 0.00 ArrayCastAndSet
1.74 11.12 0.26 60001672 0.00 0.00 pfree
1.67 11.37 0.25 10000001 0.00 0.00 slot_getattr
1.60 11.61 0.24 10000000 0.00 0.00 advance_aggregates
1.54 11.84 0.23 40000006 0.00 0.00 free_var
1.54 12.07 0.23 10000001 0.00 0.00 datumCopy
1.47 12.29 0.22 10000001 0.00 0.00 SeqNext
1.40 12.50 0.21 20000000 0.00 0.00 add_var
1.34 12.70 0.20 20000003 0.00 0.00 strip_var
1.34 12.90 0.20 10000001 0.00 0.00 ExecScan
1.27 13.09 0.19 10000003 0.00 0.00 AllocSetReset
1.20 13.27 0.18 10000003 0.00 0.00 ExecProcNode
1.13 13.44 0.17 70000010 0.00 0.00 pg_detoast_datum
0.93 13.58 0.14 10000000 0.00 0.00 numeric_avg_accum
0.93 13.72 0.14 2 0.07 6.61 ExecAgg
0.87 13.85 0.13 10000001 0.00 0.00 datumGetSize
0.87 13.98 0.13 87860 0.00 0.00 heapgetpage
0.73 14.09 0.11 10000001 0.00 0.00 DirectFunctionCall2
0.73 14.20 0.11 10000000 0.00 0.00 construct_array
0.60 14.29 0.09 10000148 0.00 0.00 DirectFunctionCall1
0.53 14.37 0.08 10000001 0.00 0.00 ExecStoreTuple
0.53 14.45 0.08 10000000 0.00 0.00 HeapTupleSatisfiesSnapshot
0.40 14.51 0.06 10000103 0.00 0.00 heap_getnext
0.33 14.56 0.05 254419 0.00 0.00 hash_search_with_hash_value
0.27 14.60 0.04 10000001 0.00 0.00 MemoryContextReset
0.27 14.64 0.04 10000000 0.00 0.00 ExecEvalVar
0.27 14.68 0.04 10000000 0.00 0.00 XidInSnapshot
0.27 14.72 0.04 511482 0.00 0.00 LWLockRelease
0.27 14.76 0.04 164939 0.00 0.00 hash_any
0.27 14.80 0.04 87760 0.00 0.00 StrategyGetBuffer
0.20 14.83 0.03 10000009 0.00 0.00 TransactionIdPrecedes
0.20 14.86 0.03 87760 0.00 0.00 FileRead
0.13 14.88 0.02 10000001 0.00 0.00 ExecSeqScan
0.13 14.90 0.02 511481 0.00 0.00 LWLockAcquire
0.13 14.92 0.02 88217 0.00 0.00 ReadBuffer
0.13 14.94 0.02 87760 0.00 0.00 TerminateBufferIO
0.07 14.95 0.01 175906 0.00 0.00 ResourceOwnerForgetBuffer
0.07 14.96 0.01 163587 0.00 0.00 get_hash_value
0.07 14.97 0.01 88019 0.00 0.00 ReleaseBuffer
0.07 14.98 0.01 87760 0.00 0.00 PinBuffer_Locked
0.00 14.98 0.00 176868 0.00 0.00 LockBuffer


---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

Neil Conway wrote:
> On Fri, 2006-11-24 at 11:08 +1300, Mark Kirkwood wrote:
>> - Modifies do_numeric_accum to have an extra bool parameter and does not
>> calc sumX2 when it is false.
>
> I think it would be clearer to reorganize this function slightly, and
> have only a single branch on "useSumX2". On first glance it isn't
> obviously that transdatums[2] is defined (but unchanged) when useSumX2
> is false.
>

Right - new patch attached that adds a new function do_numeric_avg_accum
that only uses N and sum(X). This means I could amend the avg aggregates
for numeric, int8 to have a initvalues of {0,0}.

Cheers

Mark


---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

So, if I understand this correctly, we're calling Alloc and ContextAlloc 10
times for every row being summed?

There are approx 10M rows and the profile snippet below shows 100M calls to
each of those.

- Luke


On 11/24/06 4:46 PM, "Mark Kirkwood" <markir@paradise.net.nz> wrote:

> time seconds seconds calls s/call s/call name
> 14.42 2.16 2.16 100002977 0.00 0.00 AllocSetAlloc
> 9.08 3.52 1.36 20000000 0.00 0.00 add_abs
> 5.54 4.35 0.83 10000000 0.00 0.00 slot_deform_tuple
> 5.41 5.16 0.81 60001673 0.00 0.00 AllocSetFree
> 4.34 5.81 0.65 10000000 0.00 0.00 construct_md_array
> 4.21 6.44 0.63 20000003 0.00 0.00 make_result
> 3.54 6.97 0.53 10000000 0.00 0.00 numeric_add
> 3.27 7.46 0.49 30000003 0.00 0.00 set_var_from_num
> 3.00 7.91 0.45 100002652 0.00 0.00 MemoryContextAlloc



---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

Luke Lonergan wrote:
> So, if I understand this correctly, we're calling Alloc and ContextAlloc 10
> times for every row being summed?
>
> There are approx 10M rows and the profile snippet below shows 100M calls to
> each of those.
>

Unless I've accidentally run gprof on the profile output for a 100M row
case I had lying around :-( ... I'll check

---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

Mark Kirkwood wrote:
> Luke Lonergan wrote:
>> So, if I understand this correctly, we're calling Alloc and
>> ContextAlloc 10
>> times for every row being summed?
>>
>> There are approx 10M rows and the profile snippet below shows 100M
>> calls to
>> each of those.
>>
>
> Unless I've accidentally run gprof on the profile output for a 100M row
> case I had lying around :-( ... I'll check
>

I haven't (so profile as attached is ok)...

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

---------------------------------------------------------------------------


Mark Kirkwood wrote:
> Neil Conway wrote:
> > On Fri, 2006-11-24 at 11:08 +1300, Mark Kirkwood wrote:
> >> - Modifies do_numeric_accum to have an extra bool parameter and does not
> >> calc sumX2 when it is false.
> >
> > I think it would be clearer to reorganize this function slightly, and
> > have only a single branch on "useSumX2". On first glance it isn't
> > obviously that transdatums[2] is defined (but unchanged) when useSumX2
> > is false.
> >
>
> Right - new patch attached that adds a new function do_numeric_avg_accum
> that only uses N and sum(X). This means I could amend the avg aggregates
> for numeric, int8 to have a initvalues of {0,0}.
>
> Cheers
>
> Mark


>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: Don't 'kill -9' the postmaster

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

---------------------------(end of broadcast)---------------------------
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate

I have tested this patch but it generates regression failures.

There was some code drift, so I am attaching an updated version of the
patch, and the regression diffs. The 'four' column is an 'int4' so my
guess is that somehow the wrong aggregate is being called.

---------------------------------------------------------------------------

Mark Kirkwood wrote:
> Neil Conway wrote:
> > On Fri, 2006-11-24 at 11:08 +1300, Mark Kirkwood wrote:
> >> - Modifies do_numeric_accum to have an extra bool parameter and does not
> >> calc sumX2 when it is false.
> >
> > I think it would be clearer to reorganize this function slightly, and
> > have only a single branch on "useSumX2". On first glance it isn't
> > obviously that transdatums[2] is defined (but unchanged) when useSumX2
> > is false.
> >
>
> Right - new patch attached that adds a new function do_numeric_avg_accum
> that only uses N and sum(X). This means I could amend the avg aggregates
> for numeric, int8 to have a initvalues of {0,0}.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +


---------------------------(end of broadcast)---------------------------
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate

Bruce Momjian wrote:
> I have tested this patch but it generates regression failures.
>
> There was some code drift, so I am attaching an updated version of the
> patch, and the regression diffs. The 'four' column is an 'int4' so my
> guess is that somehow the wrong aggregate is being called.
>

Good catch - I must have neglected to run the regression test after
amending the number of array arguments for the numeric avg :-(.

Hmmm - this changing the number of array args for avg means we can't mix
transition functions for variance with final functions for avg - which
is exactly what the regression suite does with the 'newavg' aggregate.

I've 'fixed' this by amending the definition of 'newavg' to use the
transition and final function that 'avg' does. However I found myself
asking if this lost us the point of that test - so I looked back at the
older postgres versions (e.g. 7.1.3) and saw that back *then* 'newavg'
and 'avg' were defined using the same functions...so I think making the
change as indicated is ok.

I've attached a new patch with this change.

Cheers

Mark


---------------------------(end of broadcast)---------------------------
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate

Mark Kirkwood wrote:
> Bruce Momjian wrote:
> > I have tested this patch but it generates regression failures.
> >
> > There was some code drift, so I am attaching an updated version of the
> > patch, and the regression diffs. The 'four' column is an 'int4' so my
> > guess is that somehow the wrong aggregate is being called.
> >
>
> Good catch - I must have neglected to run the regression test after
> amending the number of array arguments for the numeric avg :-(.
>
> Hmmm - this changing the number of array args for avg means we can't mix
> transition functions for variance with final functions for avg - which
> is exactly what the regression suite does with the 'newavg' aggregate.

Yea, I was just looking at this and came to same conclusion.

> I've 'fixed' this by amending the definition of 'newavg' to use the
> transition and final function that 'avg' does. However I found myself
> asking if this lost us the point of that test - so I looked back at the
> older postgres versions (e.g. 7.1.3) and saw that back *then* 'newavg'
> and 'avg' were defined using the same functions...so I think making the
> change as indicated is ok.
>
> I've attached a new patch with this change.

OK, great, will apply.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq