Thread: [bug fix] Memory leak in dblink

Hello,

I've fixed and tested a memory leak bug in dblink.  Could you review and
commit this?  I'll add this to the CommitFest shortly.


[Problem]
A user reported a problem in pgsql-jp ML that he encountered a "out of
memory" error when he ran the ran the following function on 32-bit
PostgreSQL 9.3:

CREATE OR REPLACE FUNCTION aaa(
character varying)
   RETURNS character varying AS
$BODY$
DECLARE
...
BEGIN
  PERFORM (SELECT DBLINK_CONNECT('conn','dbname=DB-B user=postgres'));
    DELETE FROM tbl0010 dba
      WHERE EXISTS
   (
   SELECT tbl0010_cd FROM tbl0010
   INNER JOIN (
                SELECT * FROM DBLINK
                ('conn','
  SELECT tbl0411_cd FROM tbl0411
                 INNER JOIN(
...

The above query calls dblink() hundreds of thousands of times.  You should
reproduce the problem with a simpler query like this:

CREATE TABLE mytable (col int);
INSERT INTO mytable ...;  /* insert many rows */
SELECT *
FROM mytable
WHERE EXISTS
  (SELECT *
   FROM dblink(
     'con',
     'SELECT * FROM mytable WHERE col = ' || col)
     t(col));


[Cause]
Hundreds of thousands of the following same line were output in the server
log:

dblink temporary context: 8192 total in 1 blocks; 8176 free (0 chunks); 16
used

Each dblink function call creates an instance of this memory context, but it
fails to delete it.  This bug seems to have been introduced in 9.2.0 by this
performance improvement:

Improve efficiency of dblink by using libpq's new single-row
processingmode(Kyotaro Horiguchi, Marko Kreen)


Regards
MauMau

From: "Fabrízio de Royes Mello" <fabriziomello@gmail.com>
I think there no need to add it to the commitfest, because it's a bugfix
and not a new feature. Or am I missing something?


The CommitFest app has an option "bug fix" in the list of topic choices.
I suppose the reason is that if the bug fix is only posted to pgsql-hackers 
and/or pgsql-bugs, it might be forgotten.

Regards
MauMau

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/09/2014 08:58 AM, MauMau wrote:
> From: "Fabrízio de Royes Mello" <fabriziomello@gmail.com> I think
> there no need to add it to the commitfest, because it's a bugfix 
> and not a new feature. Or am I missing something?
> 
> The CommitFest app has an option "bug fix" in the list of topic
> choices. I suppose the reason is that if the bug fix is only posted
> to pgsql-hackers and/or pgsql-bugs, it might be forgotten.

Yes, please add it to the commitfest and I'll claim it.

Thanks,

Joe


- -- 
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTlccRAAoJEDfy90M199hln1oP/i0FO8d9j6c8TAbmDHHh3p2Q
xjKvUSmnk6crAZR43M5wkUt3bj/qp58evYLJG6x0i71tJLGVjHByT2GrfFTjyWdB
hfBQy7Su1t6QsqXuOEvL4KksscRp8zGQC+vqBks69zbfi3IcfF0nAnnHzk+qWmfL
WZ7k7hhbtI03llWU7QB/JZxjKt8H1tR1kauDrQroZv0uNL4qbG6darLxt53h9WaG
0K1m/iVdrhbSYzxwMzdrvKhtYexLWA1iLje6u9lZSYhXQtTD/J+gCcSkE+VngF9I
hjVixfnWbB+Y8VF2Fwee0wbIV0C/9L1OVodFFIaGIPyLUc2bbSI9KkknK4CCfR3M
s7/mpSUPod4JKZxmNNSll/ituUV1sWq9DJ1RhiXqLU+dAxCQGTG5jxw9dHBwC0fO
giQ/srh0lnR6C3SOjgGb3mC1+uNPxNWJOt+kyL+5GIQ+RyRFBPfo5hMvlwgUnj/V
764CAJIn2IpoqEondkKRGVfJMEp3Xg/WhlXEed/hMGwoC7DT0z1GKXxWBuqJ74eA
YYAp8EeHREIs0SMcPT9qUi/iSvS3jbq6U0BQM/qRPNE6yEujJ630VXHpgwTCCcuB
37yRrGl++J91UGY3pSPCOzq14G9Mscfm4a55MD+4Svuf5lOTTAr9BLzjs6XftHev
5Mx+HaOg58xVRQBA6NVB
=F4pB
-----END PGP SIGNATURE-----

From: "Amit Kapila" <amit.kapila16@gmail.com>
> Is there a need to free memory context in PG_CATCH()?
> In error path the memory due to this temporary context will get
> freed as it will delete the transaction context and this
> temporary context will definitely be in the hierarchy of it, so
> it should also get deleted.  I think such handling can be
> useful incase we use PG_CATCH() to suppress the error.

I thought the same, but I also felt that I should make an effort to release
resources as soon as possible, considering the memory context auto deletion
as a last resort.  However, looking at other places where PG_CATCH() is
used, memory context is not deleted.  So, I removed the modification from
PG_CATCH() block.  Thanks.

Regards
MauMau

On Tue, Jun 10, 2014 at 12:27 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> Is there a need to free memory context in PG_CATCH()?
> In error path the memory due to this temporary context will get
> freed as it will delete the transaction context and this
> temporary context will definitely be in the hierarchy of it, so
> it should also get deleted.  I think such handling can be
> useful incase we use PG_CATCH() to suppress the error.

Using PG_CATCH() to suppress an error is pretty much categorically unsafe.

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

Amit Kapila <amit.kapila16@gmail.com> writes:
> On Tue, Jun 10, 2014 at 7:30 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> 
>> On Tue, Jun 10, 2014 at 12:27 AM, Amit Kapila <amit.kapila16@gmail.com>
> wrote:
>>> Is there a need to free memory context in PG_CATCH()?
>>> In error path the memory due to this temporary context will get
>>> freed as it will delete the transaction context and this
>>> temporary context will definitely be in the hierarchy of it, so
>>> it should also get deleted.  I think such handling can be
>>> useful incase we use PG_CATCH() to suppress the error.

>> Using PG_CATCH() to suppress an error is pretty much categorically unsafe.

> In some cases like for handling exceptions in plpgsql, PG_CATCH()
> is used to handle the exception and then take an appropriate action
> based on exception, so in some such cases it might be right to free
> the context memory depending on situation.

Robert's point is that the only safe way to suppress an error is to
do a (sub)transaction rollback.  That will take care of cleaning up
appropriate memory contexts, along with much else.  I don't see the
value of adding any single-purpose cleanups when they'd just be
subsumed by the transaction rollback anyhow.

The reason there's a PG_CATCH block here at all is to clean up libpq
PGresults that the transaction rollback logic won't be aware of.
We could instead have built infrastructure to allow rollback to clean
those up, but it'd be a lot more code, for not a lot of benefit given
the current complexity of dblink.c.  Maybe sometime in the future
it'll be worth doing it that way.
        regards, tom lane

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/10/2014 02:57 AM, MauMau wrote:
> From: "Amit Kapila" <amit.kapila16@gmail.com>
>> Is there a need to free memory context in PG_CATCH()? In error
>> path the memory due to this temporary context will get freed as
>> it will delete the transaction context and this temporary context
>> will definitely be in the hierarchy of it, so it should also get
>> deleted.  I think such handling can be useful incase we use
>> PG_CATCH() to suppress the error.
>
> I thought the same, but I also felt that I should make an effort
> to release resources as soon as possible, considering the memory
> context auto deletion as a last resort.  However, looking at other
> places where PG_CATCH() is used, memory context is not deleted.
> So, I removed the modification from PG_CATCH() block.  Thanks.

I think the context deletion was missed in the first place because
storeRow() is the wrong place to create the context. Rather than
creating the context in storeRow() and deleting it two levels up in
materializeQueryResult(), I think it should be created and deleted in
the interim layer, storeQueryResult(). Patch along those lines attached.

Any objections to me back-patching it this way?

Joe

- --
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTod+UAAoJEDfy90M199hlaWgP/iitSQm7m+Sf2i8hP73TAdTX
Naw+TKtag822xzVT6AoVJafeZvEv0PNRYurP69y4zE11cwiG9u+RfoO1X1hP7FYu
mdHo5++YG+znmqDVC57Qav0qAheYaCk2Y9RKEZk9gEJLUO+HJRxuzc+L277lsAni
Iyu3DyaiogzmGBtjrPPex4VMtpFAPHzHWfABaL11kvNBXXpUjO/Z02ex+1nuZDV7
+wNSV4IdTTsmpdbbi/NRhDeU7sZOerIAY29B6vI/GXfHdwhDhg+3tG4PuoDg6YG2
jX/H37UE0zq0+J3ySnM92HZtn9RxfrjdkHTz/X7DAjZjHNwBNVi18oNfrsXUyqHO
3j3VEaRelow2+tUDaTxziEkZRA0vCRoeG20B7dgQY3op60wm9lpJ+pCQH8UGwPYC
HDxaOYcVKmxbWq+j86l2ZyYlfP8sVbqTMEc00dnYoc2sdDzU36+KJadIqoMoUgds
G5uNYOZ5eTxumua9exz2cqGVOdgzcDD3r/ZAUUVM0jk3LwdA4CKLorsy26Ce59lF
mSIO58uAJe198tyOCmbpZjbypIRRO3Qm73SfUmioCbcUzSg2tDdPpf0LP62V/YEm
gFIsPHNyZnaVm0baLilbJFg+88sxFSo1hvpoaUfNPVww7woAfFxi6uBt5h5KthUO
JoDWxYjYUy9VFDdC4rh4
=gj6p
-----END PGP SIGNATURE-----

Joe Conway <mail@joeconway.com> writes:
> I think the context deletion was missed in the first place because
> storeRow() is the wrong place to create the context. Rather than
> creating the context in storeRow() and deleting it two levels up in
> materializeQueryResult(), I think it should be created and deleted in
> the interim layer, storeQueryResult(). Patch along those lines attached.

Since the storeInfo struct is longer-lived than storeQueryResult(),
it'd probably be better if the cleanup looked like

+     if (sinfo->tmpcontext != NULL)
+         MemoryContextDelete(sinfo->tmpcontext);
+    sinfo->tmpcontext = NULL;

I find myself a bit suspicious of this whole thing though.  If it's
necessary to explicitly clean up the tmpcontext, why not also the
sinfo->cstrs allocation?  And what about the tupdesc, attinmeta,
etc created further up in that "if (first)" block?  I'd have expected
that all this stuff gets allocated in a context that's short-lived
enough that we don't really need to clean it up explicitly.
        regards, tom lane

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/18/2014 12:09 PM, Tom Lane wrote:
> Joe Conway <mail@joeconway.com> writes:
>> I think the context deletion was missed in the first place
>> because storeRow() is the wrong place to create the context.
>> Rather than creating the context in storeRow() and deleting it
>> two levels up in materializeQueryResult(), I think it should be
>> created and deleted in the interim layer, storeQueryResult().
>> Patch along those lines attached.
> 
> Since the storeInfo struct is longer-lived than
> storeQueryResult(), it'd probably be better if the cleanup looked
> like
> 
> +     if (sinfo->tmpcontext != NULL) +
> MemoryContextDelete(sinfo->tmpcontext); +    sinfo->tmpcontext =
> NULL;


good point

> I find myself a bit suspicious of this whole thing though.  If
> it's necessary to explicitly clean up the tmpcontext, why not also
> the sinfo->cstrs allocation?  And what about the tupdesc,
> attinmeta, etc created further up in that "if (first)" block?  I'd
> have expected that all this stuff gets allocated in a context
> that's short-lived enough that we don't really need to clean it up
> explicitly.

Yeah, I thought about that too. My testing showed a small amount of
remaining leakage -- like 20 MB on 100,000 iterations -- I wasn't sure
that it was worthwhile to worry about. The memory context leak was
much larger.

Joe


- -- 
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJToeUQAAoJEDfy90M199hls8kP/0hhsVdn8HKcY7OZ64cKju4e
dH0SwQNMZxklpQkjEqb6vpbTmqaQ/7At/i7b6RGTMWFbIBu7L5Vpz+NZnJ7SyfaC
UjSgKfSe6M2auDIXlLCiZ62d8KZJJmVQj6U2h8ljhqorJq22kvClgXAUlcxZMoVv
SEthE1y7Udxgf3IqBO0AN6SMPrxP8bZDgrPbtZqw9UEHkCGZK0MDdH8TSRip7p6V
heyg9GbWeTvwFWBUYomMnMEUB6UlgBRnmHYsZIAjmUgxZfGiKhlydGOrb7MDHopz
ejyb06fg2MsSQnrEnCElLbonUqb5S9bD4p9BZHF/bz67AhVieJvDvnsSIoDjzR1a
+JIYe36ZDqo2kIGx4PuESgGX4ZTxxsrw033AQhVBW+KGkIuxjnvhh7tvEj3ACSjd
0bC1ztrnzCJLyNFd6jKaq5KAcNCEb/zvfKQAdHygJ87JkwE8I6u+ms1hdDpc10uZ
w484lcv/qP/JO2SaqPerDMwRsDXaovT8kEcsbqr1D7XFXu4cufHVGwe64IJYMjt+
9Y09/+i+Xt9DKXfkxC68Q2QWxwst5NfShRWDF3NG02iWlMqU2OsoVTQB6137DCiB
7hBKoNBmOApLJIjwCzEjB5IJmeeAz9x7YxgsqicRPnXoCKO+aF3dTbS+1u04va84
5XQ0p0lwUZaNfYr/xbi6
=H5bR
-----END PGP SIGNATURE-----

Joe Conway <mail@joeconway.com> writes:
> On 06/18/2014 12:09 PM, Tom Lane wrote:
>> I find myself a bit suspicious of this whole thing though.  If
>> it's necessary to explicitly clean up the tmpcontext, why not also
>> the sinfo->cstrs allocation?  And what about the tupdesc,
>> attinmeta, etc created further up in that "if (first)" block?  I'd
>> have expected that all this stuff gets allocated in a context
>> that's short-lived enough that we don't really need to clean it up
>> explicitly.

> Yeah, I thought about that too. My testing showed a small amount of
> remaining leakage -- like 20 MB on 100,000 iterations -- I wasn't sure
> that it was worthwhile to worry about. The memory context leak was
> much larger.

[ Thinks for awhile... ]  Ah.  The memory context is a child of
the econtext's ecxt_per_tuple_memory, which is supposed to be
short-lived, but we only do MemoryContextReset() on that after
each tuple, not MemoryContextResetAndDeleteChildren().  I recall
there's been debate about changing that, but it's not something
we can risk changing in back branches, for sure.  So I concur
we need an explicit context delete here.

I do see growth in the per-query context as well.  I'm not sure
where that's coming from, but we probably should try to find out.
A couple hundred bytes per iteration is going to add up, even if it's
not as fast as 8K per iteration.  I'm not sure it's dblink's fault,
because I don't see anything in dblink.c that is allocating anything in
the per-query context, except for the returned tuplestores, which
somebody else should clean up.
        regards, tom lane

I wrote:
> I do see growth in the per-query context as well.  I'm not sure
> where that's coming from, but we probably should try to find out.
> A couple hundred bytes per iteration is going to add up, even if it's
> not as fast as 8K per iteration.  I'm not sure it's dblink's fault,
> because I don't see anything in dblink.c that is allocating anything in
> the per-query context, except for the returned tuplestores, which
> somebody else should clean up.

I poked at this example some more, and found that the additional memory
leak is occurring during evaluation of the arguments to be passed to
dblink().  There's been a comment there for a very long time suggesting
that we might need to do something about that ...

With the attached patch on top of yours, I see no leak anymore.

            regards, tom lane

diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index f162e92..cf8cbb1 100644
*** a/src/backend/executor/execQual.c
--- b/src/backend/executor/execQual.c
*************** ExecMakeTableFunctionResult(ExprState *f
*** 2014,2019 ****
--- 2014,2020 ----
      PgStat_FunctionCallUsage fcusage;
      ReturnSetInfo rsinfo;
      HeapTupleData tmptup;
+     MemoryContext argContext = NULL;
      MemoryContext callerContext;
      MemoryContext oldcontext;
      bool        direct_function_call;
*************** ExecMakeTableFunctionResult(ExprState *f
*** 2083,2094 ****
          /*
           * Evaluate the function's argument list.
           *
!          * Note: ideally, we'd do this in the per-tuple context, but then the
!          * argument values would disappear when we reset the context in the
!          * inner loop.  So do it in caller context.  Perhaps we should make a
!          * separate context just to hold the evaluated arguments?
           */
          argDone = ExecEvalFuncArgs(&fcinfo, fcache->args, econtext);
          /* We don't allow sets in the arguments of the table function */
          if (argDone != ExprSingleResult)
              ereport(ERROR,
--- 2084,2104 ----
          /*
           * Evaluate the function's argument list.
           *
!          * We can't do this in the per-tuple context: the argument values
!          * would disappear when we reset that context in the inner loop.  And
!          * the caller's context is typically a query-lifespan context, so we
!          * don't want to leak memory there.  So make a separate context just
!          * to hold the evaluated arguments.
           */
+         argContext = AllocSetContextCreate(callerContext,
+                                            "Table function arguments",
+                                            ALLOCSET_DEFAULT_MINSIZE,
+                                            ALLOCSET_DEFAULT_INITSIZE,
+                                            ALLOCSET_DEFAULT_MAXSIZE);
+         oldcontext = MemoryContextSwitchTo(argContext);
          argDone = ExecEvalFuncArgs(&fcinfo, fcache->args, econtext);
+         MemoryContextSwitchTo(oldcontext);
+
          /* We don't allow sets in the arguments of the table function */
          if (argDone != ExprSingleResult)
              ereport(ERROR,
*************** no_function_result:
*** 2321,2328 ****
--- 2331,2342 ----
              FreeTupleDesc(rsinfo.setDesc);
      }

+     /* Clean up */
      MemoryContextSwitchTo(callerContext);

+     if (argContext)
+         MemoryContextDelete(argContext);
+
      /* All done, pass back the tuplestore */
      return rsinfo.setResult;
  }

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/18/2014 07:29 PM, Tom Lane wrote:
> I wrote:
>> I do see growth in the per-query context as well.  I'm not sure 
>> where that's coming from, but we probably should try to find
>> out. A couple hundred bytes per iteration is going to add up,
>> even if it's not as fast as 8K per iteration.  I'm not sure it's
>> dblink's fault, because I don't see anything in dblink.c that is
>> allocating anything in the per-query context, except for the
>> returned tuplestores, which somebody else should clean up.
> 
> I poked at this example some more, and found that the additional
> memory leak is occurring during evaluation of the arguments to be
> passed to dblink().  There's been a comment there for a very long
> time suggesting that we might need to do something about that ...
> 
> With the attached patch on top of yours, I see no leak anymore.

I can confirm that -- rock solid with 1 million iterations. I assume
that should not be back-patched though?

On a side note, while perusing this section of code:

8<-------------------------- at dblink.c:1176 --------------------------/* make sure we have a persistent copy of the
tupdesc*/tupdesc = CreateTupleDescCopy(tupdesc);
 
/* check result and tuple descriptor have the same number of columns */if (nfields != tupdesc->natts)    ereport(ERROR,
          (errcode(ERRCODE_DATATYPE_MISMATCH),             errmsg("remote query result rowtype does not match "
   "the specified FROM clause rowtype")));
 
/* Prepare attinmeta for later data conversions */sinfo->attinmeta = TupleDescGetAttInMetadata(tupdesc);
/* Create a new, empty tuplestore */oldcontext =
MemoryContextSwitchTo(rsinfo->econtext->ecxt_per_query_memory);sinfo->tuplestore= tuplestore_begin_heap(true, false,
work_mem);rsinfo->setResult= sinfo->tuplestore;rsinfo->setDesc = tupdesc;MemoryContextSwitchTo(oldcontext);
 
8<-------------------------- at dblink.c:1194 --------------------------

Shouldn't that CreateTupleDescCopy() happen in ecxt_per_query_memory?

Joe

- -- 
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTolNHAAoJEDfy90M199hlEN8QAIi3eq5D93Y4CqWWS8Uulqx1
jOBQUoLTwF7/CkYM0F+tGvX1QO1BAIDAxS6jwWXK/c9NqesVuS5tNwQcdrLb69Pm
me4hQ2ZYtoCA4Y8SiFL0sOjUGuads2QEjhL3HUMQDBXTMjCpzamotIBGvYWu1OOe
bf1VaY83bwGa6XvXYcfFFyqyUz+kMHvcM/Rq9Mj7pLrGT9Lqvec9RL/1FhFYamrI
2ewaKYC4htWXwk1xIvpDZtWTjLyv3BUl3X41BzPOecChWqXmYCpaPo+dR6V9LSm1
OxGFQL4Z3pM7VZDWk5FOj3vOFz1AJhWXEh8fyzlLKeDm7FFaApyf3rPLULBRO4sj
4C88lUdSbhzgTlMreq/wqBh2bKFmLGUA8M9sSwd+2DMc6QQQN0DWCgDtSZq/3Fwr
Tc/0LWvHwh+/Tozx0kk0DxIQzS0BOsWHzjtOMwb1p3aLZXlG9FP5IrrPJlIyDOQK
feVB9JNH5kGUFEU0OkGaqPaQy1lTVTizIA/FmTqV9QeJo2+vKSNPt2Ys4dM5jevo
Tpp6ZAjvC6sAoIoWmvtazV5WOL7FwXwf8AQRJRgmh8JqHw4C2nZt9R+CNQqbGPa2
hxiTWi5EMufBmVOJeaEX867CUTLL6qzCtr/I2a2XZyMuPD5dQbS3l2MaYw1l2ucU
o9x6G78hBR32xagpqPCE
=LqzA
-----END PGP SIGNATURE-----

Joe Conway <mail@joeconway.com> writes:
> On a side note, while perusing this section of code:

> 8<-------------------------- at dblink.c:1176 --------------------------
>  /* make sure we have a persistent copy of the tupdesc */
>  tupdesc = CreateTupleDescCopy(tupdesc);

> Shouldn't that CreateTupleDescCopy() happen in ecxt_per_query_memory?

Not necessary (we'd have seen crashes long since if it was).
ExecMakeTableFunctionResult doesn't need the tupdesc to persist past
return.

Actually, I was wondering whether we couldn't remove that
CreateTupleDescCopy call entirely.  The risk would be if
get_call_result_type returned a pointer into relcache or some other cached
tuple descriptor, which might be subject to a cache flush --- but AFAICS
it always returns a freshly created or copied tupdesc.  (This might not
have been true originally, which could explain why dblink thinks it needs
to copy.)
        regards, tom lane

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/18/2014 08:19 PM, Tom Lane wrote:
> Actually, I was wondering whether we couldn't remove that 
> CreateTupleDescCopy call entirely.

Apparently not, at least without some additional surgery.
ExecMakeTableFunctionResult() tries to free the tupledesc and segfaults.

Joe

- -- 
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTolmPAAoJEDfy90M199hlz7YQAInPRKkGLskcqx4ujmsNpbEG
RS2zSll4PqCELa/wMcWDJUsoVkzjybuw6A/1MSLGtERIamHDcmDIbTFwx9Z02n0u
nuFGgyds9auIqn+AB18rvwgas+gL6tRHOUe4bagiuqNzywnOceW98PT0NttWt86y
Fsyz/xfapIoV+kS1k9FAXC5B/PfayYoPq3cB3qWGNX/vor+xIgw3BGT9Bk3DbN2J
IqD75HqoFV5jEwiStwxLaLvgqiLPVMzI/HdiQhprbveTa1e2vFM7Tu6n02i8uFVt
fVu4UCtBtOWRIC9oOO0QhVtqnETMpwsxwWIxC5RhWScL7M8CT3Z9cw5zCIJWo2Q8
VaUDa+gpXukisg8OUfK2AhSduxcPYJ8I+b/VMS9p6j5P/87slnLuMaTnxU7afVCM
3F2UrDOgv53t43NMeD3xou8J4ntf/NJbaOsXCQx9yXjcq3UMzT0g3OSwxPbfViE+
YN6GCelnt6e0mT3Uk/pZDm0+QwYeMv6ZHjYD3y7vD4+Ipnt/HNAhO6r/HyRDk7/x
DvSeO0okJXwUqTq/xcJ6wBE7frBqTpfzLxEMLXEVMMRCZWpKOAwK05afIZsadKqP
wgeJETiSirKfWUWb0/bmMIVv2vA4fRZYpLW31pBr6OLS1GlwsrsfuYNxCm8ur1tG
gUe/trrEIMBo6iyE//N5
=i7jE
-----END PGP SIGNATURE-----

Joe Conway <mail@joeconway.com> writes:
> On 06/18/2014 08:19 PM, Tom Lane wrote:
>> Actually, I was wondering whether we couldn't remove that 
>> CreateTupleDescCopy call entirely.

> Apparently not, at least without some additional surgery.
> ExecMakeTableFunctionResult() tries to free the tupledesc and segfaults.

Hmm ... oh, I missed this bit:
           /* We must get the tupledesc from call context */           if (rsinfo && IsA(rsinfo, ReturnSetInfo) &&
        rsinfo->expectedDesc != NULL)           {               result = TYPEFUNC_COMPOSITE;               if
(resultTupleDesc)                  *resultTupleDesc = rsinfo->expectedDesc;               /* Assume no polymorphic
columnshere, either */           }
 

So it's passing back the same tupdesc passed in by the caller of
ExecMakeTableFunctionResult.  We can free that all right, but the caller
won't be happy :-(
        regards, tom lane

Joe Conway <mail@joeconway.com> writes:
> On 06/18/2014 07:29 PM, Tom Lane wrote:
>> With the attached patch on top of yours, I see no leak anymore.

> I can confirm that -- rock solid with 1 million iterations. I assume
> that should not be back-patched though?

Well, we usually think memory leaks are back-patchable bugs.  I'm
a bit worried about the potential performance impact of an extra
memory context creation/deletion though.  It's probably not noticeable in
this test case, but that's just because dblink() is such a spectacularly
expensive function.
        regards, tom lane

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/18/2014 08:45 PM, Tom Lane wrote:
> Joe Conway <mail@joeconway.com> writes:
>> On 06/18/2014 07:29 PM, Tom Lane wrote:
>>> With the attached patch on top of yours, I see no leak
>>> anymore.
> 
>> I can confirm that -- rock solid with 1 million iterations. I
>> assume that should not be back-patched though?
> 
> Well, we usually think memory leaks are back-patchable bugs.  I'm a
> bit worried about the potential performance impact of an extra 
> memory context creation/deletion though.  It's probably not
> noticeable in this test case, but that's just because dblink() is
> such a spectacularly expensive function.

Probably so. I'll try to scrounge up some time to test the performance
impact of your patch.

Joe

- -- 
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTol38AAoJEDfy90M199hleycP/2kOi2Pa6vcVXKxhNQo3mSdO
A84Ae/4LTfUbeVzUTf+uBRcz6LOtOlrHATZOcftJMlyTNmM++JJvF3YYMpGgmxO/
UfiykDs2bqDgPrIxbPxAEpgdeXWcsdJZzzOV1YWurU/qnTdoKD2ArPQhakWLGZH0
CRc46Cn2Qb3NCvnuO5R+ZhGPXS0t6EqTiGlmWtk9ZaI8MHmv1qVKMOKBor3v+2lk
/wdlc5lypPnZ07NKIjCVN0gzEJ+RV9nxQk1M3QkNYNsHOBiexEmaucXo6ab4derO
nXoOGo/0XwMhlhA6vrKlAKhxjkTNnJulVHQOWVLMCiNvcfX0KISJZVIoT/NraR94
Hc5ZZMjmhosbU8sgQiKjGoFSJq2Wld6SADuLt6xvsY9k5AiuEcPFbfVjAWlCIaEm
lOQ2cOrk+4nhEA1ygIsRw96GMT2WaEtOek4l3hJs6yd3zuzXoouO9i02QaXBqgR8
QmIJ+tOjwKnOPFThbJMjxlsrQMwJ6mPywhwt6E74YsKV6ndGFigBOfzjZlOn3OKX
DM60oWFhuCfHQdOlid1d6Uyuc4yeFb4g4XSS4sXW9wLPpvve63NxxBQ8ez0r3Up8
nLwcCqxFZRFwKX2Wp6fgtpmhgxolLF29XvpfTUMR6pPai+/Ei59vU4JXqqz0haqa
3UHpQ3AznN5vm+UvZJYe
=pvQS
-----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/18/2014 12:14 PM, Joe Conway wrote:
> On 06/18/2014 12:09 PM, Tom Lane wrote:
>> Joe Conway <mail@joeconway.com> writes:
>>> I think the context deletion was missed in the first place
>>> because storeRow() is the wrong place to create the context.
>>> Rather than creating the context in storeRow() and deleting it
>>> two levels up in materializeQueryResult(), I think it should
>>> be created and deleted in the interim layer,
>>> storeQueryResult(). Patch along those lines attached.
>
>> Since the storeInfo struct is longer-lived than
>> storeQueryResult(), it'd probably be better if the cleanup
>> looked like
>
>> +     if (sinfo->tmpcontext != NULL) +
>> MemoryContextDelete(sinfo->tmpcontext); +    sinfo->tmpcontext =
>> NULL;
>
>
> good point

If there is no further discussion, I will commit this version of the
patch back to 9.2 where this leak was introduced.

Joe

- --
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTompDAAoJEDfy90M199hlZoMP/333eXbmFvh1WCowMehKPEy1
WV3VgyZx7hBvSr/cP/HUMjXTb34hRgi5uGa79ZMyAW+U+jDxrIN4ozxp54LlgU5x
pTzD8J8VviunvWzf6stgHTe5bfcBJ9kA9oZlgHApH+JRGeySsYALI4ZBluJa1tRa
gf6ePd8Kwl4AUucbM3v7kJGxtVS5XRGKcffJnATg+OLiBUReVv9ZCjxeYasyOaRt
K2Q8ijW878UgV9HViTCsl8THelnlh7q0BpbKaSZBJG847CgmrVxfRt/l8Od8a4G/
ODoQ/fV0ZOI3h5j9oirL4/RC9HWOqchJBvBd3CK7caWLwNKVwqqEWGV3uqEO2TnA
NH3QgHb4u8WGNhoWbwL6dGWa27s2EntlWLpJRuavKxCIN2owvVBKSZ6H8d3dSlI5
AqaGnxOPvxWEB5K2w0wBynkZ9nrk4PYuIVKADu8fqyYyDsG3wGsZmI1trLNXj5sm
uE/FTbdvUjcU2uIVMMJSbPxa5JvvfR/+9rM/AF8VP5PMnSs1CyeLQXkW7nazRZOP
7zHUY6N4vwem8tVQuuXPouLu2B/eTJoXaUTm7iQvXztJDmwKxKgYCzLW/LxfvI4Q
mDIY/Arh/4RdC7kVXu6m5BEisNIbBuKtcA6f0DM+4G9i0Wtft450fgajYV4xcic9
DMPyBMwD7csz3ssF04Ox
=PJyj
-----END PGP SIGNATURE-----

From: "Joe Conway" <mail@joeconway.com>
> I think the context deletion was missed in the first place because
> storeRow() is the wrong place to create the context. Rather than
> creating the context in storeRow() and deleting it two levels up in
> materializeQueryResult(), I think it should be created and deleted in
> the interim layer, storeQueryResult(). Patch along those lines attached.
>
> Any objections to me back-patching it this way?

I thought the same at first before creating the patch, but I reconsidered. 
If the query executed by dblink() doesn't return any row, the context 
creation and deletion is a waste of processing.  I think the original author 
wanted to eliminate this waste by creating the context when dblink() should 
return a row.  I'd like to respect his thought.

Regards
MauMau

"MauMau" <maumau307@gmail.com> writes:
> From: "Joe Conway" <mail@joeconway.com>
>> Any objections to me back-patching it this way?

> I thought the same at first before creating the patch, but I reconsidered. 
> If the query executed by dblink() doesn't return any row, the context 
> creation and deletion is a waste of processing.  I think the original author 
> wanted to eliminate this waste by creating the context when dblink() should 
> return a row.  I'd like to respect his thought.

I don't think speed is really worth worrying about.  The remote query will
take orders of magnitude more time than the possibly-useless context
creation.

The code is really designed to put all the setup for storeRow into one
place; but I concur with Joe that having teardown in a different place
from setup isn't very nice.

An alternative that might be worth thinking about is putting both the
context creation and deletion at the outermost level where the storeInfo
struct is defined.  That seems possibly a shade less surprising than
having it at the intermediate level.
        regards, tom lane

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/18/2014 08:50 PM, Joe Conway wrote:
> On 06/18/2014 08:45 PM, Tom Lane wrote:
>> Well, we usually think memory leaks are back-patchable bugs.  I'm
>> a bit worried about the potential performance impact of an extra
>>  memory context creation/deletion though.  It's probably not 
>> noticeable in this test case, but that's just because dblink()
>> is such a spectacularly expensive function.
> 
> Probably so. I'll try to scrounge up some time to test the
> performance impact of your patch.

Not the most scientific of tests, but I think a reasonable one:

8<-----------
create function testcontext(arg int) returns setof int as
$$select $1$$ language sql;

do $$
declare
i int;
rec record;
begin
for i in 1..1000000 loop select * into rec from testcontext(42);
end loop;
end
$$;
8<-----------

I threw out the first run (not shown) in each group below.

without patch
- -------------
Time: 9930.320 ms
Time: 9963.114 ms
Time: 10048.067 ms
Time: 9899.810 ms
Time: 9926.066 ms
Time: 9996.044 ms
Time: 9919.095 ms
Time: 9921.482 ms
Time: 9904.839 ms
Time: 9990.285 ms
=================
Avg:  9949.912 ms

with patch
- -------------
Time: 10172.148 ms
Time: 10203.585 ms
Time: 10142.779 ms
Time: 10211.159 ms
Time: 10109.001 ms
Time: 10216.619 ms
Time: 10221.820 ms
Time: 10153.220 ms
Time: 10147.540 ms
Time: 10176.478 ms
==================
Avg:  10175.435 ms

without patch
- -------------
Time: 9897.838 ms
Time: 9848.947 ms
Time: 9830.405 ms
Time: 9824.837 ms
Time: 9828.743 ms
Time: 9990.998 ms
Time: 9820.717 ms
Time: 9853.545 ms
Time: 9850.613 ms
Time: 9836.452 ms
=================
Avg:  9858.310 ms

2.7% performance penalty

Joe

- -- 
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTowRSAAoJEDfy90M199hl2kUP+QGkKWJ8MkIOFIH7YlsGwGEK
I2jZvgTA84FBmoKuKSRJmUTXenzh3KnINHmsJxywqH3QAjYt3WFZia1OucQQgdZ5
YIpDWyN7FBS2NEwXhDSp2X/Wpqw9ZcLY1cyivUFruRTYm4viw10InNKFe3+396i/
zVt1+e0NlxJKAl4wdtk29q8rlmSJ2ej5fGATgrdd1I6C0kLhBaAxYWqMCC81JQrT
slbE/y6qeLUkyCEbvrRPj+J8rCO5sCpXvWA691x5qFSrhFaI1jE62QGq6sz4eB1F
gUBNn2c57A5sTtqZDz704FbxHAv6mXZpwb4g7jYT5bV7NBlDUxaUURoWQxLvZ9Iy
6CKZ7eM7yU0k2wpHF7bnOVt5YGtF9spd4MZOkrxSjUJ1XwdBS7IKtdymtUleTRup
5T3oFTQ/joaAGKbO3Ioq2PgcDlVgfq/x2rf/veQXV4AdlvymWamnygZ/Nf91w4QA
GpN+cOtsvLVNejqdxR4CoXWA9xu6gfmjnATaVkBQ8vQb61OGMmLmxtJWljp785zL
3jyhISMvcW2Yn7Gv07f7cV89YzfxTwl1EY34DhT9hTKXlim7qu0w0kgR4gp/MKX/
DelfTIZz1JUVwfRDwhOo3cMUGD/ru6H8N/FgtQGycXxYfLg7yK69egxpM3+oZF2t
NaEbghbhHXn4LPEbSt0L
=2yrG
-----END PGP SIGNATURE-----

Joe Conway wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 06/18/2014 08:50 PM, Joe Conway wrote:
> > On 06/18/2014 08:45 PM, Tom Lane wrote:
> >> Well, we usually think memory leaks are back-patchable bugs.  I'm
> >> a bit worried about the potential performance impact of an extra
> >>  memory context creation/deletion though.  It's probably not 
> >> noticeable in this test case, but that's just because dblink()
> >> is such a spectacularly expensive function.
> > 
> > Probably so. I'll try to scrounge up some time to test the
> > performance impact of your patch.
> 
> Not the most scientific of tests, but I think a reasonable one:

Is this an assert-enabled build?

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/19/2014 08:50 AM, Alvaro Herrera wrote:
> Joe Conway wrote:
>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
>> 
>> On 06/18/2014 08:50 PM, Joe Conway wrote:
>>> On 06/18/2014 08:45 PM, Tom Lane wrote:
>>>> Well, we usually think memory leaks are back-patchable bugs.
>>>> I'm a bit worried about the potential performance impact of
>>>> an extra memory context creation/deletion though.  It's
>>>> probably not noticeable in this test case, but that's just
>>>> because dblink() is such a spectacularly expensive function.
>>> 
>>> Probably so. I'll try to scrounge up some time to test the 
>>> performance impact of your patch.
>> 
>> Not the most scientific of tests, but I think a reasonable one:
> 
> Is this an assert-enabled build?

No:

CFLAGS = -O2 -Wall -Wmissing-prototypes -Wpointer-arith
- -Wdeclaration-after-statement -Wendif-labels
- -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
- -fwrapv -fexcess-precision=standard -g

CONFIGURE = '--prefix=/usr/local/pgsql-head' '--with-pgport=55437'
'--enable-debug' '--enable-nls' '--with-pam' '--enable-thread-safety'
'--with-openssl' '--with-krb5'
'--with-includes=/usr/include/kerberosIV'
'--with-includes=/usr/include/et' '--enable-integer-datetimes'
'--with-libxml' '' 'build_alias=' 'host_alias=' 'target_alias='
'DOCBOOKSTYLE=/usr/share/sgml/docbook/stylesheet/dsssl/modular'


- -- 
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTowsYAAoJEDfy90M199hll48QAJVSEiaYAAG4f50OyYpS5uka
y85ceg+EfE9UMG6FKbEK7z6+a0cQUumvuGOxd0TxztujHLvRCFqW+UlCALRwl5fi
d169MI+GYucHoKUj/CObM6dM8qYKK8OAVsMerpNLs97kW2Qsny1Jt930RhHxUgZe
ZyUvRsQssInMQ5JUcCy5IinENxhry9fhCMkMIIPVuCu4aF4DeIy7T/cBQkY3S+C3
5DcpHoubwcorL2lFZSpJorQ6w5bueHp0jVsWo1IfP/XlrRckaS1MIgW95YKKw/Ok
//F/CoFd9nSkyFmvZw8JW2R9o8Tv1HTZTMvzVCFK7yJGGXRQeTLEGHbYMkkaHgtr
piHf89p1wvYOMaGtjQ2pAp4oGjMQ2g3DefvmU3UiM2g9wQ9WgRBMkwnWl3IHms6g
OkozqzNtpBVyAxgoumuD2ohdnoHt521v0W4vGisLMv5ZbxHbTZBDmHBptiRkSxYH
i6jWlAnTPjXmjy335QQSMe17XAa/vE+0v3ut/vS80lGj+nAKiZnvd4XgR8VwAN8f
6qWU3E754ZnMdYaIrjdrcJ6F0L75tsvM/bS73IN+OsefiYM+BVPv4M2kiwUtOl3z
sPNl1uedv5QeF/B/69FBr6zRVBMUoxy6NTdwhZEf4aL9WnIxRO461NOlcEbBNjaC
Pds6HML5iBRZixluCHuM
=7cT8
-----END PGP SIGNATURE-----

Joe Conway <mail@joeconway.com> writes:
>> Probably so. I'll try to scrounge up some time to test the
>> performance impact of your patch.

> Not the most scientific of tests, but I think a reasonable one:
> ...
> 2.7% performance penalty

Thanks.  While that's not awful, it's enough to be annoying.

I think we could mitigate this by allocating the argument context once in
nodeFunctionscan setup, and passing it into ExecMakeTableFunctionResult;
so we'd only do a memory context reset not a create/delete in each cycle.
That would make the patch a bit more invasive, but not much.

Back-patchability would depend on whether you think there's any third
party code calling ExecMakeTableFunctionResult; I kinda doubt that,
but I wonder if anyone has a different opinion.
        regards, tom lane

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/19/2014 09:18 AM, Tom Lane wrote:
> I think we could mitigate this by allocating the argument context
> once in nodeFunctionscan setup, and passing it into
> ExecMakeTableFunctionResult; so we'd only do a memory context reset
> not a create/delete in each cycle. That would make the patch a bit
> more invasive, but not much.
> 
> Back-patchability would depend on whether you think there's any
> third party code calling ExecMakeTableFunctionResult; I kinda doubt
> that, but I wonder if anyone has a different opinion.

Seems unlikely to me.

Joe

- -- 
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTow9pAAoJEDfy90M199hlFIgQAIh8/65UIWiFzACoprPJI3O1
fLoU3mEAErCw1IE6pl+eYNiUNauvSGoizrDlqFdlcVs6PVbgirjcXYvMKNExkIoz
eKooxhb9hsfyV0iK6vDOzCWnvLIuarJJgYnu3lFJw5diEoCAywtUE8o3X3/RScQW
glMBiIuiGW2EmYkqBQTSc/lkmKIvCB9rrn1XPymvD2riTFvP466MY7gnt7vFTNjD
Cq/9PGTxdXOirSPMvIxUmZkbPOLmucDWJCl5CC8E7mYeG4XDj7YoV8KMn4zRKda9
ZUZk72BXpnsqMak0q+tHa9A9KgMkRB8Sw2YE16/qD6et+jbm1OoiD3prpBEdficZ
HQiSL/yyFjCX383ySAZ5AbfAlnnDpfWCgk9xjBWZecIt0GGNzRKBbIvO3+maAIok
XcotFHkAcFu4C0ssIJdL58tDOoqwaNshgTn9CUK6g4jqZlny9WF8RcRe6yx2XokY
x20oRcA4nZ0bDH6ZvxqBFWK0eAfc15zVH0EZaCWDYxX7LEHyr6dxNtTIj38xBt6d
RT5ioKR3k+v/mpTq8xKBedtKWsb3BHhvZ+WnncBOU/tjZJcjC6sEZ89O2rElxJ09
FalqGuioi3gR8YgfbNMAbemEwaPg8OP45ChN/SA3cEjX3OJLqHA/WZNmqO50oLZl
M3zNfviiWzHG/OjBxp3o
=8S6U
-----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/19/2014 08:18 AM, Tom Lane wrote:
> The code is really designed to put all the setup for storeRow into
> one place; but I concur with Joe that having teardown in a
> different place from setup isn't very nice.
>
> An alternative that might be worth thinking about is putting both
> the context creation and deletion at the outermost level where the
> storeInfo struct is defined.  That seems possibly a shade less
> surprising than having it at the intermediate level.

Fair enough -- this patch does it at that level in
materializeQueryResult()

Joe

- --
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJToxAAAAoJEDfy90M199hl6okP/0B5Pf+SAk7uNpLhDgE5oR27
vWpzhZHau2Yo9RkPqXwJoHRIIwluBqAjp3Ps2b9qLCPpyPFWbxll/f2K2C3LTgYi
ZcpFWQqrCdDMFbHphE642mAt8oy9xAXHsCCH4ONAUTGfhOagRKP2oKGhwbMOah74
KDfwDuyXyXEhzOjX538/3WhGXgZV5pmBUp9+QKZ8KhLy6GdAwg5h8Q2QFXy5UVuf
pZyoXP8kxuN6ubYMlRnWpUK1yxhks9Hqjahx3dU4MDrKyxPGKJL2p5jpU04an8RD
JuswYNlZpMWbaF9C1AWM7C++COWPzSx5tULOJFWNhVgmrZj6jvtKR0SVB8GK2K4G
8xepGXREj6wAzFEY3FqOmihcKi0gk6jvJ6BJocZDAB+UTSSIMvqxfj+RDtX1CYWT
YKEh5Wzs4D1bK7eKxDxWRj+EzdxJVXq6nNSUdQHwq6HwR8XcT7R7/AP6qUSl90EY
pj+8iKu+c3u/fLogqH1xcLx0d5WpDttosjvJ2d1pA6iCBTRZJofj9Q80TDSXYEn8
/gdYHjam9U11wUzNhT5n7IV+vxMdNYxlHWX076xIA0wgrz7SIAp0DBkCu844OAl7
4DJ966m3QbWUHQgzNPE0nyHpEMUOpKMTAKxxeXMDik1U/q/GUx/dekjeL09W2qvh
O//08pMr5h+y6NAjbOMQ
=9P3q
-----END PGP SIGNATURE-----

From: "Joe Conway" <mail@joeconway.com>
> Fair enough -- this patch does it at that level in
> materializeQueryResult()

I'm in favor of this.  TBH, I did this at first, but I was afraid this would 
be rejected due to the reason mentioned earlier.

if statement in PG_TRY block is not necessary like this, because sinfo is 
zero-cleared.

  PG_TRY();  {
+   /* Create short-lived memory context for data conversions */
+   sinfo.tmpcontext = AllocSetContextCreate(CurrentMemoryContext,
+            "dblink temporary context",

Regards
MauMau

Joe Conway <mail@joeconway.com> writes:
> Not the most scientific of tests, but I think a reasonable one:
> ...
> 2.7% performance penalty

I made a slightly different test based on the original example.
This uses generate_series() which is surely faster than a SQL
function, so it shows the problem to a larger degree:

create table t1 as select x from generate_series(1,10000000) x;
vacuum t1;

-- time repeated executions of this:
select count(*) from t1
where exists (select 1 from generate_series(x,x+ (random()*10)::int));

-- and this:
select count(*) from t1
where exists (select 1 from
              generate_series(x,x+ (random()*10)::int::text::int));

The first of these test queries doesn't leak memory because the argument
expressions are all pass-by-value; the second one adds some useless text
conversions just to provoke the leak (it eats about 1GB in HEAD).

HEAD, with asserts off:

first query:
Time: 21694.557 ms
Time: 21629.107 ms
Time: 21593.510 ms

second query:
Time: 26698.445 ms
Time: 26699.408 ms
Time: 26751.742 ms

With yesterday's patch:

first query:
Time: 23919.438 ms
Time: 24053.108 ms
Time: 23884.989 ms
... so about 10.7% slower, not good

second query no longer bloats, but:
Time: 29986.850 ms
Time: 29951.179 ms
Time: 29771.533 ms
... almost 12% slower

With the attached patch instead, I get:

first query:
Time: 21017.503 ms
Time: 21113.656 ms
Time: 21107.617 ms
... 2.5% faster??

second query:
Time: 27033.626 ms
Time: 26997.907 ms
Time: 26984.397 ms
... about 1% slower

In the first query, the MemoryContextReset is nearly free since there's
nothing to free (and we've special-cased that path in aset.c).  It's
certainly a measurement artifact that it measures out faster, which says
to me that these numbers can only be trusted within a couple percent;
but at least we're not taking much hit in cases where the patch isn't
actually conferring any benefit.  For the second query, losing 1% to avoid
memory bloat seems well worthwhile.

Barring objections I'll apply and back-patch this.

            regards, tom lane

diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index f162e92..bc79e3a 100644
*** a/src/backend/executor/execQual.c
--- b/src/backend/executor/execQual.c
*************** ExecMakeFunctionResultNoSets(FuncExprSta
*** 2002,2007 ****
--- 2002,2008 ----
  Tuplestorestate *
  ExecMakeTableFunctionResult(ExprState *funcexpr,
                              ExprContext *econtext,
+                             MemoryContext argContext,
                              TupleDesc expectedDesc,
                              bool randomAccess)
  {
*************** ExecMakeTableFunctionResult(ExprState *f
*** 2083,2094 ****
          /*
           * Evaluate the function's argument list.
           *
!          * Note: ideally, we'd do this in the per-tuple context, but then the
!          * argument values would disappear when we reset the context in the
!          * inner loop.  So do it in caller context.  Perhaps we should make a
!          * separate context just to hold the evaluated arguments?
           */
          argDone = ExecEvalFuncArgs(&fcinfo, fcache->args, econtext);
          /* We don't allow sets in the arguments of the table function */
          if (argDone != ExprSingleResult)
              ereport(ERROR,
--- 2084,2101 ----
          /*
           * Evaluate the function's argument list.
           *
!          * We can't do this in the per-tuple context: the argument values
!          * would disappear when we reset that context in the inner loop.  And
!          * the caller's CurrentMemoryContext is typically a query-lifespan
!          * context, so we don't want to leak memory there.  We require the
!          * caller to pass a separate memory context that can be used for this,
!          * and can be reset each time through to avoid bloat.
           */
+         MemoryContextReset(argContext);
+         oldcontext = MemoryContextSwitchTo(argContext);
          argDone = ExecEvalFuncArgs(&fcinfo, fcache->args, econtext);
+         MemoryContextSwitchTo(oldcontext);
+
          /* We don't allow sets in the arguments of the table function */
          if (argDone != ExprSingleResult)
              ereport(ERROR,
diff --git a/src/backend/executor/nodeFunctionscan.c b/src/backend/executor/nodeFunctionscan.c
index da5d8c1..945a414 100644
*** a/src/backend/executor/nodeFunctionscan.c
--- b/src/backend/executor/nodeFunctionscan.c
***************
*** 28,33 ****
--- 28,34 ----
  #include "nodes/nodeFuncs.h"
  #include "parser/parsetree.h"
  #include "utils/builtins.h"
+ #include "utils/memutils.h"


  /*
*************** FunctionNext(FunctionScanState *node)
*** 94,99 ****
--- 95,101 ----
              node->funcstates[0].tstore = tstore =
                  ExecMakeTableFunctionResult(node->funcstates[0].funcexpr,
                                              node->ss.ps.ps_ExprContext,
+                                             node->argcontext,
                                              node->funcstates[0].tupdesc,
                                            node->eflags & EXEC_FLAG_BACKWARD);

*************** FunctionNext(FunctionScanState *node)
*** 152,157 ****
--- 154,160 ----
              fs->tstore =
                  ExecMakeTableFunctionResult(fs->funcexpr,
                                              node->ss.ps.ps_ExprContext,
+                                             node->argcontext,
                                              fs->tupdesc,
                                            node->eflags & EXEC_FLAG_BACKWARD);

*************** ExecInitFunctionScan(FunctionScan *node,
*** 515,520 ****
--- 518,536 ----
      ExecAssignResultTypeFromTL(&scanstate->ss.ps);
      ExecAssignScanProjectionInfo(&scanstate->ss);

+     /*
+      * Create a memory context that ExecMakeTableFunctionResult can use to
+      * evaluate function arguments in.  We can't use the per-tuple context for
+      * this because it gets reset too often; but we don't want to leak
+      * evaluation results into the query-lifespan context either.  We just
+      * need one context, because we evaluate each function separately.
+      */
+     scanstate->argcontext = AllocSetContextCreate(CurrentMemoryContext,
+                                                   "Table function arguments",
+                                                   ALLOCSET_DEFAULT_MINSIZE,
+                                                   ALLOCSET_DEFAULT_INITSIZE,
+                                                   ALLOCSET_DEFAULT_MAXSIZE);
+
      return scanstate;
  }

diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 5e4a15c..239aff3 100644
*** a/src/include/executor/executor.h
--- b/src/include/executor/executor.h
*************** extern Datum GetAttributeByName(HeapTupl
*** 231,236 ****
--- 231,237 ----
                     bool *isNull);
  extern Tuplestorestate *ExecMakeTableFunctionResult(ExprState *funcexpr,
                              ExprContext *econtext,
+                             MemoryContext argContext,
                              TupleDesc expectedDesc,
                              bool randomAccess);
  extern Datum ExecEvalExprSwitchContext(ExprState *expression, ExprContext *econtext,
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 1f7c6d1..b271f21 100644
*** a/src/include/nodes/execnodes.h
--- b/src/include/nodes/execnodes.h
*************** typedef struct SubqueryScanState
*** 1407,1412 ****
--- 1407,1413 ----
   *        nfuncs                number of functions being executed
   *        funcstates            per-function execution states (private in
   *                            nodeFunctionscan.c)
+  *        argcontext            memory context to evaluate function arguments in
   * ----------------
   */
  struct FunctionScanPerFuncState;
*************** typedef struct FunctionScanState
*** 1421,1426 ****
--- 1422,1428 ----
      int            nfuncs;
      struct FunctionScanPerFuncState *funcstates;        /* array of length
                                                           * nfuncs */
+     MemoryContext argcontext;
  } FunctionScanState;

  /* ----------------

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/19/2014 04:36 PM, Tom Lane wrote:

> In the first query, the MemoryContextReset is nearly free since
> there's nothing to free (and we've special-cased that path in
> aset.c).  It's certainly a measurement artifact that it measures
> out faster, which says to me that these numbers can only be trusted
> within a couple percent; but at least we're not taking much hit in
> cases where the patch isn't actually conferring any benefit.  For
> the second query, losing 1% to avoid memory bloat seems well
> worthwhile.
> 
> Barring objections I'll apply and back-patch this.

Nice work! +1

Joe

- -- 
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTo4XHAAoJEDfy90M199hlEDwP/1eqjD1lu9Dk5zRCJk239I7D
hLC0DguDNs/cwPCm4tC7ngtOLBUtrLVyvWYXQw9Yxs+8JYm2rx2fpD3bq5scZfXh
mWaaIM5plYI6v1QeMtg7u8LUEuH8dw0Ycbse2oZRWwZwn9dAEBHHdQTTtrm0bCSu
cKS402hTSUYMHhyhoURN4CzK1zzmoV8kb3ZhyCx+4HQNNJ3zf/yl/fONkHdc5aar
8M66g+Emf9Qpddx8+wWUL8xcqagIU0k36tJk1lM2vT5tLQ8ezYlNwLxC674ifIZn
dlsu127uSanEegYis+lFFQriRaYl2Bs7kZfYcas33RtuTlCJN4TK5AcRmgzPs+hT
WH2Kj/cVAMd7fwrogHQzGEnEGPRRfETWj+GPL9uibgrfY6mLaV5PLEosWTIRDsq/
6aSGNrBL5jp958k+d2bNjv/WTj/LZrZTSMRA//8mc3wbae5YDFEn3o6ADvm5/3Gn
xj+ijVOYFAgnNq4P5o1TIWoWqu+GA7ExfD8FW369Hgfi58o6KKRbpM4httJz/3fH
3q3pbDHp7dNFsM5AnmjFltg97X148u6dRd7sHDMEgSmGxJQiJGLEqNyROATRVTj1
DRxxmIsVcE52Rpm0Mz7SnmoSM+1RdtVO8N9Lz9ygcokP8XbbWlXUbEKl8f3S7v+P
ANXyc09bdqXYi40afE1d
=7MYr
-----END PGP SIGNATURE-----

From: "Tom Lane" <tgl@sss.pgh.pa.us>
> In the first query, the MemoryContextReset is nearly free since there's
> nothing to free (and we've special-cased that path in aset.c).  It's
> certainly a measurement artifact that it measures out faster, which says
> to me that these numbers can only be trusted within a couple percent;
> but at least we're not taking much hit in cases where the patch isn't
> actually conferring any benefit.  For the second query, losing 1% to avoid
> memory bloat seems well worthwhile.
>
> Barring objections I'll apply and back-patch this.

So this patch would solve memory leak issues if other modules had similar 
bugs, in addition to the original dblink problem, wouldn't this?  Definitely 
+1.  The original OP wants to use 9.2.  I'll report to him when you've 
committed this nce patch.  Thanks, Tom san.

Regards
MauMau

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/19/2014 03:38 PM, MauMau wrote:
> From: "Joe Conway" <mail@joeconway.com>
>> Fair enough -- this patch does it at that level in 
>> materializeQueryResult()
> 
> I'm in favor of this.  TBH, I did this at first, but I was afraid
> this would be rejected due to the reason mentioned earlier.
> 
> if statement in PG_TRY block is not necessary like this, because
> sinfo is zero-cleared.

Right -- pushed that way, back-patched through 9.2

Joe

- -- 
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTpIv6AAoJEDfy90M199hlQtIP/ilpExgyHa86DaMCej4+oRI/
nO06AujnF+wcw/xre8xpu3sYCLebBnSbmKlqv17ry+mPfWD2KsrwMnqFgm2UoQGQ
zDB/fpz4ALfQHlWrdvBqKd/J2IcdtuWaS9xi0kqShuSXWWm4XMaUIZ+gxDAA+x4U
OXRumrv+kipHTONwMporZfby5DPtaRLpuV/o4ioOB4elb2VQAlajR5Vpmguhihdf
A6exIN6dHIKTT2jNUHfkqnd7bZ2anVaohuociM/j+JwKaRct9K+anR3bokfLKLW9
l/OQ+BHzLBDz/Pi7GB/ImmkrIKL33phbhCWWPN1nkD6OYfYohkYvl+aixZ+kvUav
wEBXTUkkJkuIL4at/5v4jbDDlPXAaYdBmGLQ/riwAOzISq2weAqcAQqhidJUbY1a
JRSgojNHbo4v8IfjEj3BMRmPlKhY6Z/eMELr2Yi+K+54Hk4Fy+UDaatyDpEo2iwm
cx2auCWBaT5SzI+KbebO0WZNhSrt7m1OEWN2tmPyTsnPsGg7I1oyQ/UtybaNjGtD
G16HIfe12wca9Sm8zu+ypnxl3gUeku5KB0ZtigNwMxZSJXm/qa4kZqGn1RoItDnh
kcfk8LTGNR1xMrPCiD+rUHyY573g8WK1XpdTWDBER29vgETdaswqXDeWsoPWmX/3
KICzxLurjqvyiJW9L4O+
=6It5
-----END PGP SIGNATURE-----