Обсуждение: 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
Вложения
* 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
* 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
* 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. +
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. +
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
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