On 22.2.2015 02:38, Jeff Davis wrote:
> On Wed, 2015-01-28 at 23:25 +0100, Tomas Vondra wrote:
>> 3) moves the assert into the 'if (release)' branch
>
> You missed one, but I got it.
Oh, thanks!
>
>> 4) includes the comments proposed by Ali Akbar in his reviews
>>
>> Warnings at makeArrayResult/makeMdArrayResult about freeing memory
>> with private subcontexts.
>
> I also edited the comments substantially.
Thanks.
>> Regarding the performance impact of decreasing the size of the
>> preallocated array from 64 to just 8 elements, I tried this.
>>
>> CREATE TABLE test AS
>> SELECT mod(i,100000) a, i FROM generate_series(1,64*100000) s(i);
>>
>> SELECT a, array_agg(i) AS x FRM test GROUP BY 1;
>>
>> or actually (to minimize transfer overhead):
>>
>> SELECT COUNT(x) FROM (
>> SELECT a, array_agg(i) AS x FRM test GROUP BY 1
>> ) foo;
>
> That's actually a bogus test -- array_agg is never executed.
Really? How could that happen when the result of array_agg() is passed
to the COUNT()? Also, how could that allocate huge amounts of memory and
get killed by OOM, which happens easily with this query?
> See the test script I used (attached). Many small groups has a
> significant improvement with your patch (median 167ms unpatched,
> 125ms patched), and the one large group is not measurably different
> (median 58ms for both).
Yup, that's expected. With a lot of small groups the improvement may
easily be much higher, but this is a conservative test.
>
> The test script uses a small dataset of 100K tuples, because 1M
> tuples will run out of memory for small groups without the patch.
>
> Committed.
Cool. Thanks once more.
regards
--
Tomas Vondra http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
