-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 06/19/2014 03:38 PM, MauMau wrote:
> From: "Joe Conway"
>> 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 re
From: "Tom Lane"
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 perc
-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,
Joe Conway 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:
From: "Joe Conway"
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
-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 wo
Joe Conway 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
-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/delet
-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
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
-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/del
"MauMau" writes:
> From: "Joe Conway"
>> 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 thi
From: "Joe Conway"
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 int
-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 writes:
>>> I think the context deletion was missed in the first place
>>> because storeRow() is the wrong place to create the context.
>>> Rather than
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 06/18/2014 08:45 PM, Tom Lane wrote:
> Joe Conway 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
>> assu
Joe Conway 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
Joe Conway 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.
-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 tuplede
Joe Conway 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() happe
-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,
>> e
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 do
Joe Conway 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
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 06/18/2014 12:09 PM, Tom Lane wrote:
> Joe Conway 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 i
Joe Conway 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
>
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 06/10/2014 02:57 AM, MauMau wrote:
> From: "Amit Kapila"
>> 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 tempora
On Wed, Jun 11, 2014 at 10:46 AM, Tom Lane wrote:
> Amit Kapila 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 might be right to free
> > the con
Amit Kapila writes:
> On Tue, Jun 10, 2014 at 7:30 PM, Robert Haas wrote:
>>
>> On Tue, Jun 10, 2014 at 12:27 AM, Amit Kapila
> 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 tr
On Tue, Jun 10, 2014 at 7:30 PM, Robert Haas wrote:
>
> On Tue, Jun 10, 2014 at 12:27 AM, Amit Kapila
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
>
On Tue, Jun 10, 2014 at 12:27 AM, Amit Kapila 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, s
From: "Amit Kapila"
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
On Mon, Jun 9, 2014 at 6:37 PM, MauMau 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 temporary context
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 06/09/2014 08:58 AM, MauMau wrote:
> From: "Fabrízio de Royes Mello" 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
From: "Fabrízio de Royes Mello"
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
On Mon, Jun 9, 2014 at 10:07 AM, MauMau 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 feature. Or am I
34 matches
Mail list logo