BUG #5608: array_agg() consumes too much memory

"Itagaki Takahiro" <itagaki.takahiro@gmail.com> writes:

I encountered "out of memory" error in large
GROUP BY query with array_agg(). The server log
was filled by the following messages:

accumArrayResult: 8192 total in 1 blocks; 7800 free (0 chunks); 392
used

Should we choose smaller size of initial memory in accumArrayResult()?

That's not really going to help much, considering that the planner's
estimated memory use per hash aggregate is only a few dozen bytes.
We have to get that estimate in sync with reality or the problem will
remain.

Eventually it might be nice to have some sort of way to specify the
estimate to use for any aggregate function --- but for a near-term
fix maybe we should just hard-wire a special case for array_agg in
count_agg_clauses_walker(). I'd be inclined to leave the array_agg
code as-is and teach the planner to assume ALLOCSET_DEFAULT_INITSIZE
per array_agg aggregate.

regards, tom lane

2010/8/10 Tom Lane <tgl@sss.pgh.pa.us>:

Eventually it might be nice to have some sort of way to specify the
estimate to use for any aggregate function --- but for a near-term
fix maybe we should just hard-wire a special case for array_agg in
count_agg_clauses_walker().

The attached patch is the near-term fix; it adds ALLOCSET_DEFAULT_INITSIZE
bytes to memory assumption.

We might need the same adjustment for string_agg(), that consumes
1024 bytes for the transit condition. array_agg() and string_agg()
are only aggregates that have "internal" for aggtranstype.

--
Itagaki Takahiro

2010/8/14 Itagaki Takahiro <itagaki.takahiro@gmail.com>:

2010/8/10 Tom Lane <tgl@sss.pgh.pa.us>:

Eventually it might be nice to have some sort of way to specify the
estimate to use for any aggregate function --- but for a near-term
fix maybe we should just hard-wire a special case for array_agg in
count_agg_clauses_walker().

The attached patch is the near-term fix; it adds ALLOCSET_DEFAULT_INITSIZE
bytes to memory assumption.

We might need the same adjustment for string_agg(), that consumes
1024 bytes for the transit condition. array_agg() and string_agg()
are only aggregates that have "internal" for aggtranstype.

So, is it better to generalize as it adds ALLOCSET_DEFAULT_INITSIZE if
the transtype is internal, rather than specifying individual function
OID as the patch stands?

Regards,

--
Hitoshi Harada

Hitoshi Harada <umi.tanuki@gmail.com> writes:

2010/8/14 Itagaki Takahiro <itagaki.takahiro@gmail.com>:

2010/8/10 Tom Lane <tgl@sss.pgh.pa.us>:

Eventually it might be nice to have some sort of way to specify the
estimate to use for any aggregate function --- but for a near-term
fix maybe we should just hard-wire a special case for array_agg in
count_agg_clauses_walker().

So, is it better to generalize as it adds ALLOCSET_DEFAULT_INITSIZE if
the transtype is internal, rather than specifying individual function
OID as the patch stands?

Seems like a good idea ... it's ugly, but it seems much less likely to
need maintenance.

regards, tom lane

Hitoshi Harada <umi.tanuki@gmail.com> writes:

2010/8/14 Itagaki Takahiro <itagaki.takahiro@gmail.com>:

The attached patch is the near-term fix; it adds ALLOCSET_DEFAULT_INITSIZE
bytes to memory assumption.

We might need the same adjustment for string_agg(), that consumes
1024 bytes for the transit condition. array_agg() and string_agg()
are only aggregates that have "internal" for aggtranstype.

So, is it better to generalize as it adds ALLOCSET_DEFAULT_INITSIZE if
the transtype is internal, rather than specifying individual function
OID as the patch stands?

I've applied a patch following Hitoshi-san's idea.

BTW, a note about the upthread patch: we don't do manual #define's for
OIDs in pg_proc.h. Use fmgroids.h for that.

regards, tom lane