Missing pfree in logical_heap_rewrite_flush_mappings()
It seems to me that when flushing logical mappings to disk, each
mapping file leaks the buffer used to pass the mappings to XLogInsert.
Also, it seems consistent to allocate that buffer in the RewriteState
memory context. Patch attached.
Regards,
Ants Aasma
--
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de
Attachments:
missing-pfree-in-logical_heap_rewrite_flush_mappings.patchtext/x-patch; charset=US-ASCII; name=missing-pfree-in-logical_heap_rewrite_flush_mappings.patchDownload
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 4cf07ea..ae439e8 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -897,7 +897,7 @@ logical_heap_rewrite_flush_mappings(RewriteState state)
/* write all mappings consecutively */
len = src->num_mappings * sizeof(LogicalRewriteMappingData);
- waldata = palloc(len);
+ waldata = MemoryContextAlloc(state->rs_cxt, len);
waldata_start = waldata;
/*
@@ -943,6 +943,7 @@ logical_heap_rewrite_flush_mappings(RewriteState state)
/* write xlog record */
XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_REWRITE, rdata);
+ pfree(waldata);
}
Assert(state->rs_num_rewrite_mappings == 0);
}
* Ants Aasma (ants@cybertec.at) wrote:
It seems to me that when flushing logical mappings to disk, each
mapping file leaks the buffer used to pass the mappings to XLogInsert.
Also, it seems consistent to allocate that buffer in the RewriteState
memory context. Patch attached.
Hmm, yeah, it does look that way. Why bother pfree'ing it here though
instead of letting it be cleaned up with state->rs_cxt in
end_heap_rewrite()?
Thanks,
Stephen
On 2014-03-26 12:49:41 -0400, Stephen Frost wrote:
* Ants Aasma (ants@cybertec.at) wrote:
It seems to me that when flushing logical mappings to disk, each
mapping file leaks the buffer used to pass the mappings to XLogInsert.
Also, it seems consistent to allocate that buffer in the RewriteState
memory context. Patch attached.
Good catch. There's actually no need for explicitly using the context,
we're in the appropriate one. The only other MemoryContextAlloc() caller
in there should be converted to a palloc as well.
Hmm, yeah, it does look that way. Why bother pfree'ing it here though
instead of letting it be cleaned up with state->rs_cxt in
end_heap_rewrite()?
For a somewhat large relation (say a pg_attribute in a db with lots of
tables), this can actually get to be a somewhat significant amount of
memory. It *will* currently already get cleaned up with the context, but
we can easily do better.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Andres Freund (andres@2ndquadrant.com) wrote:
On 2014-03-26 12:49:41 -0400, Stephen Frost wrote:
* Ants Aasma (ants@cybertec.at) wrote:
It seems to me that when flushing logical mappings to disk, each
mapping file leaks the buffer used to pass the mappings to XLogInsert.
Also, it seems consistent to allocate that buffer in the RewriteState
memory context. Patch attached.Good catch. There's actually no need for explicitly using the context,
we're in the appropriate one. The only other MemoryContextAlloc() caller
in there should be converted to a palloc as well.
Hrm..? I don't think that's right when it's called from
end_heap_rewrite(). Perhaps we should be switching to state->rs_cxt
while in end_heap_rewrite() also though?
Hmm, yeah, it does look that way. Why bother pfree'ing it here though
instead of letting it be cleaned up with state->rs_cxt in
end_heap_rewrite()?For a somewhat large relation (say a pg_attribute in a db with lots of
tables), this can actually get to be a somewhat significant amount of
memory. It *will* currently already get cleaned up with the context, but
we can easily do better.
Ok, so perhaps we should go ahead and pfree() this as we go.
Thanks,
Stephen
On 2014-03-26 13:41:27 -0400, Stephen Frost wrote:
* Andres Freund (andres@2ndquadrant.com) wrote:
On 2014-03-26 12:49:41 -0400, Stephen Frost wrote:
* Ants Aasma (ants@cybertec.at) wrote:
It seems to me that when flushing logical mappings to disk, each
mapping file leaks the buffer used to pass the mappings to XLogInsert.
Also, it seems consistent to allocate that buffer in the RewriteState
memory context. Patch attached.Good catch. There's actually no need for explicitly using the context,
we're in the appropriate one. The only other MemoryContextAlloc() caller
in there should be converted to a palloc as well.Hrm..? I don't think that's right when it's called from
end_heap_rewrite().
You're right. Apprently I shouldn't look at patches while watching a
keynote ;)
Perhaps we should be switching to state->rs_cxt
while in end_heap_rewrite() also though?
I think just applying Aant's patch is fine.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Andres Freund (andres@2ndquadrant.com) wrote:
On 2014-03-26 13:41:27 -0400, Stephen Frost wrote:
Good catch. There's actually no need for explicitly using the context,
we're in the appropriate one. The only other MemoryContextAlloc() caller
in there should be converted to a palloc as well.Hrm..? I don't think that's right when it's called from
end_heap_rewrite().You're right. Apprently I shouldn't look at patches while watching a
keynote ;)
No problem- but that's also why I was thinking we should just wrap
end_heap_rewrite() in a context as well, otherwise the next person who
comes along to add a bit of code here may end up making the same
mistake. Is there something you're concerned about there?
Perhaps we should be switching to state->rs_cxt
while in end_heap_rewrite() also though?I think just applying Aant's patch is fine.
I have no problem *also* doing the pfree() to address the concern about
the transient memory usage being more than we'd like.
Thanks,
Stephen
On Wed, Mar 26, 2014 at 06:29:38PM +0200, Ants Aasma wrote:
It seems to me that when flushing logical mappings to disk, each
mapping file leaks the buffer used to pass the mappings to XLogInsert.
Also, it seems consistent to allocate that buffer in the RewriteState
memory context. Patch attached.
Patch applied.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Apr 22, 2014 at 06:05:53PM -0400, Bruce Momjian wrote:
On Wed, Mar 26, 2014 at 06:29:38PM +0200, Ants Aasma wrote:
It seems to me that when flushing logical mappings to disk, each
mapping file leaks the buffer used to pass the mappings to XLogInsert.
Also, it seems consistent to allocate that buffer in the RewriteState
memory context. Patch attached.Patch applied.
I had to revert this patch. It causes a failure in the
/contrib/test_decoding regression test.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Bruce Momjian <bruce@momjian.us> writes:
I had to revert this patch. It causes a failure in the
/contrib/test_decoding regression test.
On closer inspection, it was simply pfree'ing the wrong pointer.
I fixed that and also undid the allocation in a different memory
context, which didn't seem to be a particularly good idea, unless
you've got a specific reason why CurrentMemoryContext would be the
wrong place for a transient allocation.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2014-04-22 22:37:37 -0400, Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
I had to revert this patch. It causes a failure in the
/contrib/test_decoding regression test.On closer inspection, it was simply pfree'ing the wrong pointer.
Thanks for fixing.
I fixed that and also undid the allocation in a different memory
context, which didn't seem to be a particularly good idea, unless
you've got a specific reason why CurrentMemoryContext would be the
wrong place for a transient allocation.
That should be fine. I don't see any reason not to use palloc.
logical_rewrite_log_mapping() has to allocate longer living memory, I
guess it was copied from there.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers