From: Tom Lane t...@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
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 06/19/2014 03:38 PM, MauMau wrote:
From: Joe Conway m...@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
From: Joe Conway m...@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
MauMau maumau...@gmail.com writes:
From: Joe Conway m...@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
-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
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
-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
-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
Joe Conway m...@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
-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
From: Joe Conway m...@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
Joe Conway m...@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
-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,
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 06/10/2014 02:57 AM, MauMau wrote:
From: Amit Kapila amit.kapil...@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
Joe Conway m...@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
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 06/18/2014 12:09 PM, Tom Lane wrote:
Joe Conway m...@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()
Joe Conway m...@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
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
-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
Joe Conway m...@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
-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
Joe Conway m...@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
Joe Conway m...@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
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 06/18/2014 08:45 PM, Tom Lane wrote:
Joe Conway m...@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
-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 m...@joeconway.com writes:
I think the context deletion was missed in the first place
because storeRow() is the wrong place to create the context.
From: Amit Kapila amit.kapil...@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
On Tue, Jun 10, 2014 at 12:27 AM, Amit Kapila amit.kapil...@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
On Tue, Jun 10, 2014 at 7:30 PM, Robert Haas robertmh...@gmail.com wrote:
On Tue, Jun 10, 2014 at 12:27 AM, Amit Kapila amit.kapil...@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
Amit Kapila amit.kapil...@gmail.com writes:
On Tue, Jun 10, 2014 at 7:30 PM, Robert Haas robertmh...@gmail.com wrote:
On Tue, Jun 10, 2014 at 12:27 AM, Amit Kapila amit.kapil...@gmail.com
wrote:
Is there a need to free memory context in PG_CATCH()?
In error path the memory due to this
On Wed, Jun 11, 2014 at 10:46 AM, Tom Lane t...@sss.pgh.pa.us wrote:
Amit Kapila amit.kapil...@gmail.com writes:
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
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
On Mon, Jun 9, 2014 at 10:07 AM, MauMau maumau...@gmail.com wrote:
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.
I think there no need to add it to the commitfest, because it's a bugfix
and not a new
From: Fabrízio de Royes Mello fabriziome...@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
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 06/09/2014 08:58 AM, MauMau wrote:
From: Fabrízio de Royes Mello fabriziome...@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
On Mon, Jun 9, 2014 at 6:37 PM, MauMau maumau...@gmail.com wrote:
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.
Is there a need to free memory context in PG_CATCH()?
In error path the memory due to this
35 matches
Mail list logo
