BUG #16112: large, unexpected memory consumption

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

BUG #16112: large, unexpected memory consumption

PG Bug reporting form
The following bug has been logged on the website:

Bug reference:      16112
Logged by:          Ben Cornett
Email address:      [hidden email]
PostgreSQL version: 12.0
Operating system:   linux 2.6.32
Description:        

Creation of table t1 in the query below causes the server process to consume
close to 1GB of memory.  The amount of memory consumed is proportional to
the value passed to generate_series in the first query.

CREATE temp TABLE t0 AS
SELECT
        i,
        ARRAY[1,2,3] a
FROM GENERATE_SERIES(1, 12000000) i
;

CREATE TEMP TABLE t1 AS
SELECT
        i,
        x
FROM t0, UNNEST(a) x;

I observed the same behavior in other test queries that included implicit
lateral joins.

Thanks very much,
Ben

Reply | Threaded
Open this post in threaded view
|

Re: BUG #16112: large, unexpected memory consumption

Tomas Vondra-4
On Wed, Nov 13, 2019 at 12:22:07PM +0000, PG Bug reporting form wrote:

>The following bug has been logged on the website:
>
>Bug reference:      16112
>Logged by:          Ben Cornett
>Email address:      [hidden email]
>PostgreSQL version: 12.0
>Operating system:   linux 2.6.32
>Description:
>
>Creation of table t1 in the query below causes the server process to consume
>close to 1GB of memory.  The amount of memory consumed is proportional to
>the value passed to generate_series in the first query.
>
>CREATE temp TABLE t0 AS
>SELECT
>        i,
>        ARRAY[1,2,3] a
>FROM GENERATE_SERIES(1, 12000000) i
>;
>
>CREATE TEMP TABLE t1 AS
>SELECT
>        i,
>        x
>FROM t0, UNNEST(a) x;
>
>I observed the same behavior in other test queries that included implicit
>lateral joins.
>
Yeah, I can reproduce this pretty easily. It seems like a memory leak in
ExecMakeTableFunctionResult. a9c35cf85ca reworked FunctionCallInfo to be
variable-length, but it gets allocated in ExecutorState context directly
and so until the end of the query.

The attached trivial patch fixes that by adding a pfree() at the end of
the function. I wonder if we have the same issue elsewhere ...


regards

--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

memleak-a9c35cf85ca.patch (376 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: BUG #16112: large, unexpected memory consumption

Andres Freund
Hi,

On 2019-11-13 15:50:04 +0100, Tomas Vondra wrote:
> Yeah, I can reproduce this pretty easily. It seems like a memory leak in
> ExecMakeTableFunctionResult. a9c35cf85ca reworked FunctionCallInfo to be
> variable-length, but it gets allocated in ExecutorState context directly
> and so until the end of the query.

Damn. Too bad this got discovered just after the point release was
wrapped :(


> The attached trivial patch fixes that by adding a pfree() at the end of
> the function.

Hm. That's clearly an improvement. But I'm not quite sure it's really
the right direction. It seems like a bad idea to rely on
ExecMakeTableFunctionResult() otherwise never leaking any memory.

It seems to we should be using memory contexts to provide a backstop
against leaks, like we normally do, instead of operating in the
per-query context. It's noteworthy that nodeProjectSet already does so.
In contrast to nodeProjectSet, I think we'd need an additional memory
context however, as we eagerly evaluate ValuePerCall expressions - and
we'd like to reset the context after each iteration.


I think we also ought to use SetExprState->fcinfo, instead of allocating
a separate allocation in ExecMakeTableFunctionResult(). But that's
perhaps less important.


> I wonder if we have the same issue elsewhere ...

Quite possible - but I think in most cases we are using memory contexts
to protect against leaks like this (which is more efficient than retail
pfree'ing).

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: BUG #16112: large, unexpected memory consumption

Tom Lane-2
Andres Freund <[hidden email]> writes:
> On 2019-11-13 15:50:04 +0100, Tomas Vondra wrote:
>> The attached trivial patch fixes that by adding a pfree() at the end of
>> the function.

> Hm. That's clearly an improvement. But I'm not quite sure it's really
> the right direction. It seems like a bad idea to rely on
> ExecMakeTableFunctionResult() otherwise never leaking any memory.

Considering that ExecMakeTableFunctionResult went from zero pallocs
to one, I don't see a strong reason why it should have bothered with
a private memory context before, nor do I think that's a good
response now.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: BUG #16112: large, unexpected memory consumption

Tomas Vondra-4
In reply to this post by Andres Freund
On Wed, Nov 13, 2019 at 09:05:28AM -0800, Andres Freund wrote:

>Hi,
>
>On 2019-11-13 15:50:04 +0100, Tomas Vondra wrote:
>> Yeah, I can reproduce this pretty easily. It seems like a memory leak in
>> ExecMakeTableFunctionResult. a9c35cf85ca reworked FunctionCallInfo to be
>> variable-length, but it gets allocated in ExecutorState context directly
>> and so until the end of the query.
>
>Damn. Too bad this got discovered just after the point release was
>wrapped :(
>
>
>> The attached trivial patch fixes that by adding a pfree() at the end of
>> the function.
>
>Hm. That's clearly an improvement. But I'm not quite sure it's really
>the right direction. It seems like a bad idea to rely on
>ExecMakeTableFunctionResult() otherwise never leaking any memory.
>
>It seems to we should be using memory contexts to provide a backstop
>against leaks, like we normally do, instead of operating in the
>per-query context. It's noteworthy that nodeProjectSet already does so.
>In contrast to nodeProjectSet, I think we'd need an additional memory
>context however, as we eagerly evaluate ValuePerCall expressions - and
>we'd like to reset the context after each iteration.
>

Possibly, but I think most of the allocations already use the per-tuple
context. It's just the fcinfo that seems to be allocated outside of it,
so maybe we should just move it to that memory context.

>
>I think we also ought to use SetExprState->fcinfo, instead of allocating
>a separate allocation in ExecMakeTableFunctionResult(). But that's
>perhaps less important.
>

Maybe.

>
>> I wonder if we have the same issue elsewhere ...
>
>Quite possible - but I think in most cases we are using memory contexts
>to protect against leaks like this (which is more efficient than retail
>pfree'ing).
>

Yeah, probably. I had a quick look at some of those places (found by
looking for SizeForFunctionCallInfo and palloc on the same line) and
those that I looked at seemed fine.

regards

--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: BUG #16112: large, unexpected memory consumption

Andres Freund
In reply to this post by Tom Lane-2
Hi,

On 2019-11-13 12:21:47 -0500, Tom Lane wrote:

> Andres Freund <[hidden email]> writes:
> > On 2019-11-13 15:50:04 +0100, Tomas Vondra wrote:
> >> The attached trivial patch fixes that by adding a pfree() at the end of
> >> the function.
>
> > Hm. That's clearly an improvement. But I'm not quite sure it's really
> > the right direction. It seems like a bad idea to rely on
> > ExecMakeTableFunctionResult() otherwise never leaking any memory.
>
> Considering that ExecMakeTableFunctionResult went from zero pallocs
> to one, I don't see a strong reason why it should have bothered with
> a private memory context before, nor do I think that's a good
> response now.

Well, that relies on type_is_rowtype(), exprType(),
TupleDescInitEntry(), lookup_rowtype_tupdesc_copy() not to leak memory
(besides lookup_rowtype_tupdesc_copy()'s return type, which is
explicitly freed). Which is true, but still seems not super reliable.

Thinking about it for a second longer, I don't think we'd need a new
context - afaict argcontext has exactly the lifetime requirements
needed.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: BUG #16112: large, unexpected memory consumption

Tom Lane-2
Andres Freund <[hidden email]> writes:
> Thinking about it for a second longer, I don't think we'd need a new
> context - afaict argcontext has exactly the lifetime requirements
> needed.

Hm, maybe so.  That'd definitely be a better solution if it works.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: BUG #16112: large, unexpected memory consumption

Jeff Janes
In reply to this post by Tomas Vondra-4
On Wed, Nov 13, 2019 at 9:50 AM Tomas Vondra <[hidden email]> wrote:

Yeah, I can reproduce this pretty easily. It seems like a memory leak in
ExecMakeTableFunctionResult. a9c35cf85ca reworked FunctionCallInfo to be
variable-length, but it gets allocated in ExecutorState context directly
and so until the end of the query.

I find the leak was introduced much earlier than that, in:

commit 763f2edd92095b1ca2f4476da073a28505c13820
Author: Andres Freund <[hidden email]>
Date:   Thu Nov 15 14:26:14 2018 -0800

    Rejigger materializing and fetching a HeapTuple from a slot.
 
I have no idea if this info is useful to informing the best solution, though.

You patch applied to REL_12_STABLE does fix it for me.


The attached trivial patch fixes that by adding a pfree() at the end of
the function. I wonder if we have the same issue elsewhere ...


Is there an easy way to assess if the "make check" regression tests are taking more memory than they used to?

Cheers,

Jeff 
Reply | Threaded
Open this post in threaded view
|

Re: BUG #16112: large, unexpected memory consumption

Andres Freund
Hi,

On 2019-11-13 17:53:28 -0500, Jeff Janes wrote:

> On Wed, Nov 13, 2019 at 9:50 AM Tomas Vondra <[hidden email]>
> wrote:
>
> >
> > Yeah, I can reproduce this pretty easily. It seems like a memory leak in
> > ExecMakeTableFunctionResult. a9c35cf85ca reworked FunctionCallInfo to be
> > variable-length, but it gets allocated in ExecutorState context directly
> > and so until the end of the query.
> >
>
> I find the leak was introduced much earlier than that, in:
>
> commit 763f2edd92095b1ca2f4476da073a28505c13820
> Author: Andres Freund <[hidden email]>
> Date:   Thu Nov 15 14:26:14 2018 -0800
>
>     Rejigger materializing and fetching a HeapTuple from a slot.
>
> I have no idea if this info is useful to informing the best solution,
> though.
>
> You patch applied to REL_12_STABLE does fix it for me.

I think that's likely two overlapping issues.

Possibly
commit 88e6ad3054ddd5aa0dee12e5def2c335fe92a414
Author: Andres Freund <[hidden email]>
Date:   2019-04-19 11:33:37 -0700

    Fix two memory leaks around force-storing tuples in slots.



> > The attached trivial patch fixes that by adding a pfree() at the end of
> > the function. I wonder if we have the same issue elsewhere ...
> >
> >
> Is there an easy way to assess if the "make check" regression tests are
> taking more memory than they used to?

Not easily, I think. I also suspect that you'd not have seen a
meaningful increase in memory usage due to this bug - it "only" was
noticable for the query at hand, because the FunctionScan node was
reached so many times, due to the lateral join forcing it to be scanned
once for each value in the source temp table. I don't think we have a
query in the tests doing so often enough (partially because we don't,
partially because it'd take a lot of time on slow machines).

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: BUG #16112: large, unexpected memory consumption

Tomas Vondra-4
In reply to this post by Jeff Janes
On Wed, Nov 13, 2019 at 05:53:28PM -0500, Jeff Janes wrote:

>On Wed, Nov 13, 2019 at 9:50 AM Tomas Vondra <[hidden email]>
>wrote:
>
>>
>> Yeah, I can reproduce this pretty easily. It seems like a memory leak in
>> ExecMakeTableFunctionResult. a9c35cf85ca reworked FunctionCallInfo to be
>> variable-length, but it gets allocated in ExecutorState context directly
>> and so until the end of the query.
>>
>
>I find the leak was introduced much earlier than that, in:
>
>commit 763f2edd92095b1ca2f4476da073a28505c13820
>Author: Andres Freund <[hidden email]>
>Date:   Thu Nov 15 14:26:14 2018 -0800
>
>    Rejigger materializing and fetching a HeapTuple from a slot.
>
>I have no idea if this info is useful to informing the best solution,
>though.
>

Ah, I've only done a simple 'git blame' and assumed it's the last commit
that touched the palloc line (and it seemed somewhat plausible). I don't
think it changes the reasoning too much, though.

>You patch applied to REL_12_STABLE does fix it for me.
>
>
>> The attached trivial patch fixes that by adding a pfree() at the end of
>> the function. I wonder if we have the same issue elsewhere ...
>>
>>
>Is there an easy way to assess if the "make check" regression tests are
>taking more memory than they used to?
>

Probably not. The regression tests use fairly small number of rows in
general, and we don't have a way to track/inspect high watermaks or
anything like that anyway.

regards

--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: BUG #16112: large, unexpected memory consumption

Andres Freund
In reply to this post by Tom Lane-2
Hi,

On 2019-11-13 13:03:31 -0500, Tom Lane wrote:
> Andres Freund <[hidden email]> writes:
> > Thinking about it for a second longer, I don't think we'd need a new
> > context - afaict argcontext has exactly the lifetime requirements
> > needed.
>
> Hm, maybe so.  That'd definitely be a better solution if it works.

Here's a patch doing so. I think it'd be a good idea to rename
argcontext into something like setcontext or such. But as I'm not that
happy with the name, I've not yet made that change.

Greetings,

Andres Freund

leakplug.diff (3K) Download Attachment