Re: memory leak when serializing TRUNCATE in reorderbuffer

On 08/08/2018 09:47 PM, Tomas Vondra wrote:
> 
> IMHO the cleanest way is to add a method like
> ReorderBufferGetChange, which does the allocation internally. That
> way the memory context choice is up to reorderbuffer, not decode.c.
> That's at least consistent with what the rest of decode.c does.
> 

OK, attached is a patch doing it along these lines. It introduces
ReorderBufferGetRelids and ReorderBufferReturnRelids, which make the
decision which context to use (so it's contained in reorderbuffer.c).
Currently the main reorderbuffer context is used - we can't use the SLAB
contexts, and the tup_context is meant for something else.

I've considered adding yet another special-purpose context into
reorderbuffer, but it seems like an overkill considering that TRUNCATE
usually is not particularly common operation. So using the main context
seems OK.

I plan to commit this over the next couple of days, and backpatch it to
pg11 where TRUNCATE decoding was introduced. It's clearly a memory leak,
and there is no behavior change.

Two more things I noticed while working on this:

1) We're using very different data types for nrelids on various places
in the decoding. xl_heap_truncate uses uint32, logicalrep_write_truncate
uses int and ReorderBufferChange uses Size. That seems somewhat strange,
although it's unlikely to overflow (Who would truncate that many rels at
the same time?).

2) The tup_context is allocated like this:

  buffer->tup_context = GenerationContextCreate(new_ctx,
                                                "Tuples",
                                                SLAB_LARGE_BLOCK_SIZE);

Using SLAB_ constant for GenerationContextCreate is not a bug, but it's
a bit confusing.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services