possible memory leak with SRFs

Hi,

I saw this behavior with latest GIT head:

create table xlarge(val numeric(19,0));
insert into xlarge values(generate_series(1,5));

The above generate series will return an int8 which will then be
casted to numeric (via int8_to_numericvar) before being inserted into
the table. I observed that the ExprContext memory associated with
econtext->ecxt_per_tuple_memory is slowly bloating up till the end of
the insert operation.

This becomes significant the moment we try to insert a significant
number of entries using this SRF. I can see the memory being consumed
by the PG backend slowly grow to a large percentage.

I see that the executor (take ExecResult as an example) does not reset
the expression context early if an SRF is churning out tuples. What
could be a good way to fix this?

Regards,
Nikhils
--
http://www.enterprisedb.com

Hi,

Continuing on this:

Can someone please explain why we do not reset the expression context
if an SRF is involved during execution? Once the current result from
the SRF has been consumed, I would think that the
ecxt_per_tuple_memory context should be reset. As its name suggests,
it is supposed to a per tuple context and is not meant to be
long-lived. To test this out I shifted the call to ResetExprContext to
just before returning from the SRF inside ExecResult and I do not see
the memleak at all. Patch attached with this mail.

The SRF has its own long-lived "SRF multi-call context" anyways. And
AIUI, SRFs return tuples one-by-one or do we materialize the same into
a tuplestore in some cases?

Regards,
Nikhils

On Wed, May 5, 2010 at 7:23 PM, Nikhil Sontakke
<nikhil(dot)sontakke(at)enterprisedb(dot)com> wrote:
> Hi,
>
> I saw this behavior with latest GIT head:
>
> create table xlarge(val numeric(19,0));
> insert into xlarge values(generate_series(1,5));
>
> The above generate series will return an int8 which will then be
> casted to numeric (via int8_to_numericvar) before being inserted into
> the table. I observed that the ExprContext memory associated with
> econtext->ecxt_per_tuple_memory is slowly bloating up till the end of
> the insert operation.
>
> This becomes significant the moment we try to insert a significant
> number of entries using this SRF. I can see the memory being consumed
> by the PG backend slowly grow to a large percentage.
>
> I see that the executor (take ExecResult as an example) does not reset
> the expression context early if an SRF is churning out tuples. What
> could be a good way to fix this?
>
> Regards,
> Nikhils
> --
> http://www.enterprisedb.com
>

--
http://www.enterprisedb.com

> Patch attached with this mail.
>

The previous patch was WIP. Please take a look at this one instead. I
hope this handles the cases where results are not just base datums but
palloc'ed datums like string types too.

Regards,
Nikhils

> The SRF has its own long-lived "SRF multi-call context" anyways. And
> AIUI, SRFs return tuples one-by-one or do we materialize the same into
> a tuplestore in some cases?
>
> Regards,
> Nikhils
>
> On Wed, May 5, 2010 at 7:23 PM, Nikhil Sontakke
> <nikhil(dot)sontakke(at)enterprisedb(dot)com> wrote:
>> Hi,
>>
>> I saw this behavior with latest GIT head:
>>
>> create table xlarge(val numeric(19,0));
>> insert into xlarge values(generate_series(1,5));
>>
>> The above generate series will return an int8 which will then be
>> casted to numeric (via int8_to_numericvar) before being inserted into
>> the table. I observed that the ExprContext memory associated with
>> econtext->ecxt_per_tuple_memory is slowly bloating up till the end of
>> the insert operation.
>>
>> This becomes significant the moment we try to insert a significant
>> number of entries using this SRF. I can see the memory being consumed
>> by the PG backend slowly grow to a large percentage.
>>
>> I see that the executor (take ExecResult as an example) does not reset
>> the expression context early if an SRF is churning out tuples. What
>> could be a good way to fix this?
>>
>> Regards,
>> Nikhils
>> --
>> http://www.enterprisedb.com
>>
>
>
>
> --
> http://www.enterprisedb.com
>

--
http://www.enterprisedb.com

Nikhil Sontakke <nikhil(dot)sontakke(at)enterprisedb(dot)com> writes:
> Can someone please explain why we do not reset the expression context
> if an SRF is involved during execution?

Consider
srf(foo(col))
where foo returns a pass-by-reference datatype. Your proposed patch
would cut the knees out from under argument values that the SRF could
reasonably expect to still be there on subsequent calls.

regards, tom lane

Hi,

>> Can someone please explain why we do not reset the expression context
>> if an SRF is involved during execution?
>
> Consider
>        srf(foo(col))
> where foo returns a pass-by-reference datatype.  Your proposed patch
> would cut the knees out from under argument values that the SRF could
> reasonably expect to still be there on subsequent calls.
>

Yeah this is my basic confusion. But wouldn't the arguments be
evaluated afresh on the subsequent call for this SRF? In that case
freeing up the context of the *last* call should not be an issue I
would think.

And if this is indeed the case we should be using a different longer
lived context and not the ecxt_per_tuple_memory context..

Regards,
Nikhils
--
http://www.enterprisedb.com

Nikhil Sontakke <nikhil(dot)sontakke(at)enterprisedb(dot)com> writes:
>> Consider
>> srf(foo(col))
>> where foo returns a pass-by-reference datatype.

> Yeah this is my basic confusion. But wouldn't the arguments be
> evaluated afresh on the subsequent call for this SRF?

No, see ExecMakeFunctionResult(). If we did that we'd have serious
problems with volatile functions, ie srf(random()).

regards, tom lane

>> Yeah this is my basic confusion. But wouldn't the arguments be
>> evaluated afresh on the subsequent call for this SRF?
>
> No, see ExecMakeFunctionResult().  If we did that we'd have serious
> problems with volatile functions, ie srf(random()).
>

Ok thanks. So if someone uses a really long-running srf with argument
expression evaluations thrown in, then running into "out of memory"
issues should be expected and then in those cases they are better off
using multiple srf calls to get the same effect if they can..

Regards,
Nikhils
--
http://www.enterprisedb.com

Nikhil Sontakke <nikhil(dot)sontakke(at)enterprisedb(dot)com> writes:
> Ok thanks. So if someone uses a really long-running srf with argument
> expression evaluations thrown in, then running into "out of memory"
> issues should be expected and then in those cases they are better off
> using multiple srf calls to get the same effect if they can..

I believe this is only an issue for SRFs called in a query targetlist,
which is a usage fraught with semantic problems anyway. Hopefully we
can get LATERAL done soon (I'm planning to look at it for 9.1) and
then deprecate this whole mess.

regards, tom lane

On 05/07/2010 09:06 PM, Nikhil Sontakke wrote:
>>> Yeah this is my basic confusion. But wouldn't the arguments be
>>> evaluated afresh on the subsequent call for this SRF?
>>
>> No, see ExecMakeFunctionResult(). If we did that we'd have serious
>> problems with volatile functions, ie srf(random()).
>
> Ok thanks. So if someone uses a really long-running srf with argument
> expression evaluations thrown in, then running into "out of memory"
> issues should be expected and then in those cases they are better off
> using multiple srf calls to get the same effect if they can..

I've very recently looked into this exact case myself for someone, and
came to the conclusion that there is no simple fix for this. If you want
to see a concrete example of a query that fails, apply your patch and
then run the regression tests -- the "misc" test will fail.

I think this is an example of why we still need to implement a real
SFRM_ValuePerCall mode that allows results to be pipelined. Yes,
ValuePerCall sort of works from the targetlist, but it is pretty much
useless for the use cases where people really want to use it.

Or would a FROM clause ValuePerCall suffer the same issue?

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> I think this is an example of why we still need to implement a real
> SFRM_ValuePerCall mode that allows results to be pipelined. Yes,
> ValuePerCall sort of works from the targetlist, but it is pretty much
> useless for the use cases where people really want to use it.

> Or would a FROM clause ValuePerCall suffer the same issue?

I don't think it'd be a big problem. We could use the technique
suggested in the comments in ExecMakeTableFunctionResult: use a separate
memory context for evaluating the arguments than for evaluating the
function itself. This will work in FROM because we can insist the SRF
be at top level. The problem with SRFs in tlists is that they can be
anywhere and there can be more than one, so it's too hard to keep track
of what to reset when.

regards, tom lane

On 05/08/2010 09:12 AM, Tom Lane wrote:
> Joe Conway <mail(at)joeconway(dot)com> writes:
>> I think this is an example of why we still need to implement a real
>> SFRM_ValuePerCall mode that allows results to be pipelined. Yes,
>> ValuePerCall sort of works from the targetlist, but it is pretty much
>> useless for the use cases where people really want to use it.
>
>> Or would a FROM clause ValuePerCall suffer the same issue?
>
> I don't think it'd be a big problem. We could use the technique
> suggested in the comments in ExecMakeTableFunctionResult: use a separate
> memory context for evaluating the arguments than for evaluating the
> function itself. This will work in FROM because we can insist the SRF
> be at top level. The problem with SRFs in tlists is that they can be
> anywhere and there can be more than one, so it's too hard to keep track
> of what to reset when.

That's what I was thinking. I saw your other email about LATERAL for 9.1
-- would it be helpful for me to work on this issue for 9.1? After all,
about 7 years ago I said I'd do it ;-). Or do you think it will be an
integral part of the LATERAL work?

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> That's what I was thinking. I saw your other email about LATERAL for 9.1
> -- would it be helpful for me to work on this issue for 9.1? After all,
> about 7 years ago I said I'd do it ;-). Or do you think it will be an
> integral part of the LATERAL work?

Dunno. What I was actually wanting to focus on is the problem of
generalizing nestloop inner indexscans so that they can use parameters
coming from more than one join level up. Robert suggested that LATERAL
might fall out of that fairly easily, but I haven't thought much about
it yet.

regards, tom lane