Subject: Re: memory leak when serializing TRUNCATE in reorderbuffer
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
 pgsql-hackers@lists.postgresql.org
References: <66175a41-9342-2845-652f-1bd4c3ee50aa@2ndquadrant.com>
 <04cfcf19-1a43-5d85-918d-56a8620377c4@2ndquadrant.com>
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Message-ID: <a6e5cf90-bee9-8b57-5f13-bed59cf6014f@2ndquadrant.com>
Date: Wed, 8 Aug 2018 21:47:17 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.2
MIME-Version: 1.0
In-Reply-To: <04cfcf19-1a43-5d85-918d-56a8620377c4@2ndquadrant.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 08/08/2018 09:19 PM, Peter Eisentraut wrote:
> On 20/06/2018 21:42, Tomas Vondra wrote:
>> So I think we should fix and serialize/restore the OID array, just like
>> we do for tuples, snapshots etc. See the attached fix.
> 
> Yes please.
> 

OK, will do.

>> Another thing we should probably reconsider is where the relids is
>> allocated - the pointer remains valid because we happen to allocate it
>> in TopMemoryContext. It's not that bad because we don't free the other
>> reorderbuffer contexts until the walsender exits anyway, but still.
>>
>> So I propose to allocate it in rb->context just like the other bits of
>> data (snapshots, ...). Replacing the palloc() in DecodeTruncate() with
>> something like:
>>
>>      MemoryContextAlloc(ctx->reorder->context,
>>                         xlrec->nrelids * sizeof(Oid));
>>
>> should do the trick.
> 
> It's not clear from the code comments which context would be the
> appropriate one.
> 
> More standard coding style would be to set the current memory context
> somewhere, but I suppose the reorderbuffer.c code isn't written that way.
> 

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.

regards

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