Re: [HACKERS] memory leak in e94568ecc10 (pre-reading in external sort)

2016-10-11 Thread Heikki Linnakangas
On 10/11/2016 12:56 AM, Peter Geoghegan wrote: Also, something about trace_sort here: +#ifdef TRACE_SORT + if (trace_sort) + elog(LOG, "using " INT64_FORMAT " KB of memory for read buffers among %d input tapes", +(state->availMem) / 1024, numInputTapes); +#endif + +

Re: [HACKERS] memory leak in e94568ecc10 (pre-reading in external sort)

2016-10-10 Thread Peter Geoghegan
On Mon, Oct 10, 2016 at 5:21 AM, Heikki Linnakangas wrote: > Admittedly that's confusing. Thinking about this some more, I came up with > the attached. I removed the separate LogicalTapeAssignReadBufferSize() call > altogether - the read buffer size is now passed as argument to

Re: [HACKERS] memory leak in e94568ecc10 (pre-reading in external sort)

2016-10-10 Thread Heikki Linnakangas
On 10/06/2016 06:44 PM, Peter Geoghegan wrote: While the fix you pushed was probably a good idea anyway, I still think you should not use swhichtate->maxTapes to exhaustively call LogicalTapeAssignReadBufferSize() on every tape, even non-active tapes. That's the confusing part. Admittedly

Re: [HACKERS] memory leak in e94568ecc10 (pre-reading in external sort)

2016-10-06 Thread Peter Geoghegan
On Thu, Oct 6, 2016 at 8:44 AM, Peter Geoghegan wrote: > Besides, what I propose to do is really exactly the same as what you > also want to do, except it avoids actually changing state->maxTapes. > We'd just pass down what you propose to assign to state->maxTapes > directly,

Re: [HACKERS] memory leak in e94568ecc10 (pre-reading in external sort)

2016-10-06 Thread Peter Geoghegan
On Thu, Oct 6, 2016 at 12:00 AM, Heikki Linnakangas wrote: > This is related to earlier the discussion with Peter G, on whether we should > change state->maxTapes to reflect the actual number of tape that were used, > when that's less than maxTapes. I think his confusion about

Re: [HACKERS] memory leak in e94568ecc10 (pre-reading in external sort)

2016-10-06 Thread Heikki Linnakangas
On 10/06/2016 07:50 AM, Tomas Vondra wrote: it seems e94568ecc10 has a pretty bad memory leak. A simple Oops, fixed, thanks for the report! To be precise, this wasn't a memory leak, just a gross overallocation of memory. The new code in tuplesort.c assumes that it's harmless to call