Re: pgsql: Improve memory management for external sorts.

Improve memory management for external sorts.

Introduce a new memory context which stores tuple data, and reset it
at the end of each merge pass; this helps avoid memory fragmentation
and, consequently, overallocation. Also, for the final merge patch,
eliminate memory context chunk header overhead entirely by allocating
all of the memory used for buffering tuples during the merge in a
single chunk. Since this modestly increases the number of tuples we
can store, grow the memtuples array a bit so that we're less likely to
run short of slots there.

Peter Geoghegan. Review and testing of patches in this series by
Jeff Janes, Greg Stark, Mithun Cy, and me.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/0011c0091e886b874e485a46ff2c94222ffbf550

Modified Files
--------------
src/backend/utils/sort/tuplesort.c | 556 ++++++++++++++++++++++++++++++++++---
1 file changed, 516 insertions(+), 40 deletions(-)

There is a typo in a comment. See attachment.

There is also a typo in commit message: s/management/management/. But it
is my understanding that we don't fix such things.

> There is also a typo in commit message: s/management/management/

Oops. I meant "mangement" :)

commit c27033ff7c17b5100d02c454a0eebb95ec7b91cc
Author: Robert Haas <rhaas(at)postgresql(dot)org>
Date: Thu Mar 17 16:11:14 2016 -0400

Update tuplesort.c comments for memory mangement improvements.

--
Best regards,
Aleksander Alekseev
http://eax.me/

On 2016-03-17 20:11:00 +0000, Robert Haas wrote:
> Improve memory management for external sorts.
>
> Introduce a new memory context which stores tuple data, and reset it
> at the end of each merge pass; this helps avoid memory fragmentation
> and, consequently, overallocation. Also, for the final merge patch,
> eliminate memory context chunk header overhead entirely by allocating
> all of the memory used for buffering tuples during the merge in a
> single chunk. Since this modestly increases the number of tuples we
> can store, grow the memtuples array a bit so that we're less likely to
> run short of slots there.
>
> Peter Geoghegan. Review and testing of patches in this series by
> Jeff Janes, Greg Stark, Mithun Cy, and me.

Cross compiling for windows results in:
/home/andres/src/postgresql/src/backend/utils/sort/tuplesort.c: In function ‘beginmerge’:
/home/andres/src/postgresql/src/backend/utils/sort/tuplesort.c:2695:34: warning: format ‘%ld’ expects argument of type ‘long int’, but argument 4 has type ‘int64 {aka long long int}’ [-Wformat=]
elog(LOG, "tape %d initially used %ld KB of %ld KB batch "
^
/home/andres/src/postgresql/src/backend/utils/sort/tuplesort.c:2695:34: warning: format ‘%ld’ expects argument of type ‘long int’, but argument 5 has type ‘int64 {aka long long int}’ [-Wformat=]
config.status: creating src/interfaces/ecpg/include/ecpg_config.h

/home/andres/src/postgresql/src/backend/utils/sort/tuplesort.c: In
function ‘beginmerge’:
/home/andres/src/postgresql/src/backend/utils/sort/tuplesort.c:2695:34:
warning: format ‘%ld’ expects argument of type ‘long int’, but argument
4 has type ‘int64 {aka long long int}’ [-Wformat=]
elog(LOG, "tape %d initially used %ld KB of %ld KB batch "

Which seems like a valid complain on a LLP64 platform (i.e. where long
is 32bit) like windows.

Andres

On Fri, Mar 18, 2016 at 2:25 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-03-17 20:11:00 +0000, Robert Haas wrote:
>> Improve memory management for external sorts.
>>
>> Introduce a new memory context which stores tuple data, and reset it
>> at the end of each merge pass; this helps avoid memory fragmentation
>> and, consequently, overallocation. Also, for the final merge patch,
>> eliminate memory context chunk header overhead entirely by allocating
>> all of the memory used for buffering tuples during the merge in a
>> single chunk. Since this modestly increases the number of tuples we
>> can store, grow the memtuples array a bit so that we're less likely to
>> run short of slots there.
>>
>> Peter Geoghegan. Review and testing of patches in this series by
>> Jeff Janes, Greg Stark, Mithun Cy, and me.
>
> Cross compiling for windows results in:
> /home/andres/src/postgresql/src/backend/utils/sort/tuplesort.c: In function ‘beginmerge’:
> /home/andres/src/postgresql/src/backend/utils/sort/tuplesort.c:2695:34: warning: format ‘%ld’ expects argument of type ‘long int’, but argument 4 has type ‘int64 {aka long long int}’ [-Wformat=]
> elog(LOG, "tape %d initially used %ld KB of %ld KB batch "
> ^
> /home/andres/src/postgresql/src/backend/utils/sort/tuplesort.c:2695:34: warning: format ‘%ld’ expects argument of type ‘long int’, but argument 5 has type ‘int64 {aka long long int}’ [-Wformat=]
> config.status: creating src/interfaces/ecpg/include/ecpg_config.h
>
> /home/andres/src/postgresql/src/backend/utils/sort/tuplesort.c: In
> function ‘beginmerge’:
> /home/andres/src/postgresql/src/backend/utils/sort/tuplesort.c:2695:34:
> warning: format ‘%ld’ expects argument of type ‘long int’, but argument
> 4 has type ‘int64 {aka long long int}’ [-Wformat=]
> elog(LOG, "tape %d initially used %ld KB of %ld KB batch "
>
> Which seems like a valid complain on a LLP64 platform (i.e. where long
> is 32bit) like windows.

Oops. Thanks for the report. Does this fix it?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Mar 18, 2016 at 2:39 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Mar 18, 2016 at 2:25 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> On 2016-03-17 20:11:00 +0000, Robert Haas wrote:
>>> Improve memory management for external sorts.
>>>
>>> Introduce a new memory context which stores tuple data, and reset it
>>> at the end of each merge pass; this helps avoid memory fragmentation
>>> and, consequently, overallocation. Also, for the final merge patch,
>>> eliminate memory context chunk header overhead entirely by allocating
>>> all of the memory used for buffering tuples during the merge in a
>>> single chunk. Since this modestly increases the number of tuples we
>>> can store, grow the memtuples array a bit so that we're less likely to
>>> run short of slots there.
>>>
>>> Peter Geoghegan. Review and testing of patches in this series by
>>> Jeff Janes, Greg Stark, Mithun Cy, and me.
>>
>> Cross compiling for windows results in:
>> /home/andres/src/postgresql/src/backend/utils/sort/tuplesort.c: In function ‘beginmerge’:
>> /home/andres/src/postgresql/src/backend/utils/sort/tuplesort.c:2695:34: warning: format ‘%ld’ expects argument of type ‘long int’, but argument 4 has type ‘int64 {aka long long int}’ [-Wformat=]
>> elog(LOG, "tape %d initially used %ld KB of %ld KB batch "
>> ^
>> /home/andres/src/postgresql/src/backend/utils/sort/tuplesort.c:2695:34: warning: format ‘%ld’ expects argument of type ‘long int’, but argument 5 has type ‘int64 {aka long long int}’ [-Wformat=]
>> config.status: creating src/interfaces/ecpg/include/ecpg_config.h
>>
>> /home/andres/src/postgresql/src/backend/utils/sort/tuplesort.c: In
>> function ‘beginmerge’:
>> /home/andres/src/postgresql/src/backend/utils/sort/tuplesort.c:2695:34:
>> warning: format ‘%ld’ expects argument of type ‘long int’, but argument
>> 4 has type ‘int64 {aka long long int}’ [-Wformat=]
>> elog(LOG, "tape %d initially used %ld KB of %ld KB batch "
>>
>> Which seems like a valid complain on a LLP64 platform (i.e. where long
>> is 32bit) like windows.
>
> Oops. Thanks for the report. Does this fix it?

Trying again to attach the patch.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016-03-18 14:40:33 -0400, Robert Haas wrote:
> > Oops. Thanks for the report. Does this fix it?
>
> Trying again to attach the patch.

Yes, that removes the warning, and looks correct.

Andres Freund

On Fri, Mar 18, 2016 at 11:43 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> Yes, that removes the warning, and looks correct.

Thanks. We should be careful to not repeat this mistake when the
quicksort patch goes in.

--
Peter Geoghegan

On Fri, Mar 18, 2016 at 4:22 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Fri, Mar 18, 2016 at 11:43 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> Yes, that removes the warning, and looks correct.
>
> Thanks. We should be careful to not repeat this mistake when the
> quicksort patch goes in.

It would be helpful if you could either (a) confirm that that patch
still applies and that it has no issues of this type or (b) post an
updated version.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, Mar 19, 2016 at 6:38 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> It would be helpful if you could either (a) confirm that that patch
> still applies and that it has no issues of this type or (b) post an
> updated version.

I don't think that it has a problem with lacking the right int64
format specifiers. However, I had a bad feeling about integer overflow
of state->currentRun, and think I'll need to address that. After all,
if runs are now no longer 2x work_mem on average, it's not completely
ridiculous to imagine that being an issue on a misconfigured system.

--
Peter Geoghegan