Re: [HACKERS] Missing pfree in logical_heap_rewrite_flush_mappings()
On 2014-04-22 22:37:37 -0400, Tom Lane wrote: > Bruce Momjian 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
Re: [HACKERS] Missing pfree in logical_heap_rewrite_flush_mappings()
Bruce Momjian 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
Re: [HACKERS] Missing pfree in logical_heap_rewrite_flush_mappings()
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 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
Re: [HACKERS] Missing pfree in logical_heap_rewrite_flush_mappings()
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 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
Re: [HACKERS] Missing pfree in logical_heap_rewrite_flush_mappings()
* Andres Freund (and...@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 signature.asc Description: Digital signature
Re: [HACKERS] Missing pfree in logical_heap_rewrite_flush_mappings()
On 2014-03-26 13:41:27 -0400, Stephen Frost wrote: > * Andres Freund (and...@2ndquadrant.com) wrote: > > On 2014-03-26 12:49:41 -0400, Stephen Frost wrote: > > > * Ants Aasma (a...@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
Re: [HACKERS] Missing pfree in logical_heap_rewrite_flush_mappings()
* Andres Freund (and...@2ndquadrant.com) wrote: > On 2014-03-26 12:49:41 -0400, Stephen Frost wrote: > > * Ants Aasma (a...@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 signature.asc Description: Digital signature
Re: [HACKERS] Missing pfree in logical_heap_rewrite_flush_mappings()
On 2014-03-26 12:49:41 -0400, Stephen Frost wrote: > * Ants Aasma (a...@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
Re: [HACKERS] Missing pfree in logical_heap_rewrite_flush_mappings()
* Ants Aasma (a...@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 signature.asc Description: Digital signature
[HACKERS] 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 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); } -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
