pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedmemory leak when serializing TRUNCATE in reorderbuffer
5+ messages / 2 participants
[nested] [flat]
Commits from this thread:
2018-09-03 4ddd8f5f55 081cf78d12 Fix memory leak in TRUNCATE decoding
* memory leak when serializing TRUNCATE in reorderbuffer
@ 2018-06-20 19:42 Tomas Vondra <tomas.vondra@2ndquadrant.com>
2018-08-08 19:19 ` Re: memory leak when serializing TRUNCATE in reorderbuffer Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
0 siblings, 1 reply; 5+ messages in thread
From: Tomas Vondra @ 2018-06-20 19:42 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
Hi,
While rebasing the logical replication patches on top of PG11, I've
noticed that ReorderBufferSerializeChange claims this:
case REORDER_BUFFER_CHANGE_TRUNCATE:
...
/* ReorderBufferChange contains everything important */
That is not quite correct, though - the OIDs of truncated relations is
stored in a separately palloc-ed array. So we only serialize the pointer
to that array (which is part of ReorderBufferChange) and then read it
back when restoring the change from disk.
Now, this can't cause crashes, because the 'relids' array won't go away
(more on this later), and so the pointer remains valid. But it's a
memory leak - a quite small and not very common one, because people
don't do TRUNCATE very often, particularly not with many tables.
So I think we should fix and serialize/restore the OID array, just like
we do for tuples, snapshots etc. See the attached fix.
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. The other places in decode.c don't allocate memory
directly but call ReorderBufferGetTupleBuf() instead - perhaps we should
introduce a similar wrapper here too.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
truncate-spill-fix.patch (text/x-patch, 2.2K) download
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index e2f59bf580..582bedf1de 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -412,10 +412,14 @@ ReorderBufferReturnChange(ReorderBuffer *rb, ReorderBufferChange *change)
}
break;
/* no data in addition to the struct itself */
+ case REORDER_BUFFER_CHANGE_TRUNCATE:
+ if (change->data.truncate.relids != NULL)
+ pfree(change->data.truncate.relids);
+ change->data.truncate.relids = NULL;
+ break;
case REORDER_BUFFER_CHANGE_INTERNAL_SPEC_CONFIRM:
case REORDER_BUFFER_CHANGE_INTERNAL_COMMAND_ID:
case REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID:
- case REORDER_BUFFER_CHANGE_TRUNCATE:
break;
}
@@ -2289,6 +2293,26 @@ ReorderBufferSerializeChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
break;
}
case REORDER_BUFFER_CHANGE_TRUNCATE:
+ {
+ Size size;
+ char *data;
+
+ /* account for the OIDs of truncated relations */
+ size = sizeof(Oid) * change->data.truncate.nrelids;
+ sz += size;
+
+ /* make sure we have enough space */
+ ReorderBufferSerializeReserve(rb, sz);
+
+ data = ((char *) rb->outbuf) + sizeof(ReorderBufferDiskChange);
+ /* might have been reallocated above */
+ ondisk = (ReorderBufferDiskChange *) rb->outbuf;
+
+ memcpy(data, change->data.truncate.relids, size);
+ data += size;
+
+ break;
+ }
case REORDER_BUFFER_CHANGE_INTERNAL_SPEC_CONFIRM:
case REORDER_BUFFER_CHANGE_INTERNAL_COMMAND_ID:
case REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID:
@@ -2569,6 +2593,16 @@ ReorderBufferRestoreChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
}
/* the base struct contains all the data, easy peasy */
case REORDER_BUFFER_CHANGE_TRUNCATE:
+ {
+ Size size;
+
+ size = change->data.truncate.nrelids * sizeof(Oid);
+
+ change->data.truncate.relids = MemoryContextAllocZero(rb->context, size);
+
+ memcpy(change->data.truncate.relids, data, size);
+ break;
+ }
case REORDER_BUFFER_CHANGE_INTERNAL_SPEC_CONFIRM:
case REORDER_BUFFER_CHANGE_INTERNAL_COMMAND_ID:
case REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID:
* Re: memory leak when serializing TRUNCATE in reorderbuffer
2018-06-20 19:42 memory leak when serializing TRUNCATE in reorderbuffer Tomas Vondra <tomas.vondra@2ndquadrant.com>
@ 2018-08-08 19:19 ` Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
2018-08-08 19:47 ` Re: memory leak when serializing TRUNCATE in reorderbuffer Tomas Vondra <tomas.vondra@2ndquadrant.com>
0 siblings, 1 reply; 5+ messages in thread
From: Peter Eisentraut @ 2018-08-08 19:19 UTC (permalink / raw)
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>; pgsql-hackers@lists.postgresql.org
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.
> 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.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: memory leak when serializing TRUNCATE in reorderbuffer
2018-06-20 19:42 memory leak when serializing TRUNCATE in reorderbuffer Tomas Vondra <tomas.vondra@2ndquadrant.com>
2018-08-08 19:19 ` Re: memory leak when serializing TRUNCATE in reorderbuffer Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
@ 2018-08-08 19:47 ` Tomas Vondra <tomas.vondra@2ndquadrant.com>
2018-08-16 12:24 ` Re: memory leak when serializing TRUNCATE in reorderbuffer Tomas Vondra <tomas.vondra@2ndquadrant.com>
0 siblings, 1 reply; 5+ messages in thread
From: Tomas Vondra @ 2018-08-08 19:47 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: memory leak when serializing TRUNCATE in reorderbuffer
2018-06-20 19:42 memory leak when serializing TRUNCATE in reorderbuffer Tomas Vondra <tomas.vondra@2ndquadrant.com>
2018-08-08 19:19 ` Re: memory leak when serializing TRUNCATE in reorderbuffer Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
2018-08-08 19:47 ` Re: memory leak when serializing TRUNCATE in reorderbuffer Tomas Vondra <tomas.vondra@2ndquadrant.com>
@ 2018-08-16 12:24 ` Tomas Vondra <tomas.vondra@2ndquadrant.com>
2018-09-03 00:35 ` Re: memory leak when serializing TRUNCATE in reorderbuffer Tomas Vondra <tomas.vondra@2ndquadrant.com>
0 siblings, 1 reply; 5+ messages in thread
From: Tomas Vondra @ 2018-08-16 12:24 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; pgsql-hackers@lists.postgresql.org
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
Attachments:
truncate-decoding-v2.patch (text/x-patch, 4.8K) download
diff --git a/src/backend/replication/logical/decode.c b/src/backend/replication/logical/decode.c
index 59c003de9c..6e46884194 100644
--- a/src/backend/replication/logical/decode.c
+++ b/src/backend/replication/logical/decode.c
@@ -859,7 +859,7 @@ DecodeTruncate(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
if (xlrec->flags & XLH_TRUNCATE_RESTART_SEQS)
change->data.truncate.restart_seqs = true;
change->data.truncate.nrelids = xlrec->nrelids;
- change->data.truncate.relids = palloc(xlrec->nrelids * sizeof(Oid));
+ change->data.truncate.relids = ReorderBufferGetRelids(ctx->reorder, xlrec->nrelids);
memcpy(change->data.truncate.relids, xlrec->relids,
xlrec->nrelids * sizeof(Oid));
ReorderBufferQueueChange(ctx->reorder, XLogRecGetXid(r),
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 1d43a165ad..0e41040ac2 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -409,10 +409,16 @@ ReorderBufferReturnChange(ReorderBuffer *rb, ReorderBufferChange *change)
}
break;
/* no data in addition to the struct itself */
+ case REORDER_BUFFER_CHANGE_TRUNCATE:
+ if (change->data.truncate.relids != NULL)
+ {
+ ReorderBufferReturnRelids(rb, change->data.truncate.relids);
+ change->data.truncate.relids = NULL;
+ }
+ break;
case REORDER_BUFFER_CHANGE_INTERNAL_SPEC_CONFIRM:
case REORDER_BUFFER_CHANGE_INTERNAL_COMMAND_ID:
case REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID:
- case REORDER_BUFFER_CHANGE_TRUNCATE:
break;
}
@@ -451,6 +457,35 @@ ReorderBufferReturnTupleBuf(ReorderBuffer *rb, ReorderBufferTupleBuf *tuple)
}
/*
+ * Get an array for relids of truncated relations. We use the global context
+ * for the whole reorder buffer - none of the existing ones seems like a good
+ * match (change/txn are SLAB, so we can't use them anyway, and tup_context
+ * is meant for tuple data, not for relids). We could add yet another context,
+ * but it seems like an overkill. So just use rb->context directly.
+ */
+Oid *
+ReorderBufferGetRelids(ReorderBuffer *rb, int nrelids)
+{
+ Oid *relids;
+ Size alloc_len;
+
+ alloc_len = sizeof(Oid) * nrelids;
+
+ relids = (Oid *) MemoryContextAlloc(rb->context, alloc_len);
+
+ return relids;
+}
+
+/*
+ * Free an array of relids.
+ */
+void
+ReorderBufferReturnRelids(ReorderBuffer *rb, Oid *relids)
+{
+ pfree(relids);
+}
+
+/*
* Return the ReorderBufferTXN from the given buffer, specified by Xid.
* If create is true, and a transaction doesn't already exist, create it
* (with the given LSN, and as top transaction if that's specified);
@@ -2412,6 +2447,26 @@ ReorderBufferSerializeChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
break;
}
case REORDER_BUFFER_CHANGE_TRUNCATE:
+ {
+ Size size;
+ char *data;
+
+ /* account for the OIDs of truncated relations */
+ size = sizeof(Oid) * change->data.truncate.nrelids;
+ sz += size;
+
+ /* make sure we have enough space */
+ ReorderBufferSerializeReserve(rb, sz);
+
+ data = ((char *) rb->outbuf) + sizeof(ReorderBufferDiskChange);
+ /* might have been reallocated above */
+ ondisk = (ReorderBufferDiskChange *) rb->outbuf;
+
+ memcpy(data, change->data.truncate.relids, size);
+ data += size;
+
+ break;
+ }
case REORDER_BUFFER_CHANGE_INTERNAL_SPEC_CONFIRM:
case REORDER_BUFFER_CHANGE_INTERNAL_COMMAND_ID:
case REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID:
@@ -2695,6 +2750,16 @@ ReorderBufferRestoreChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
}
/* the base struct contains all the data, easy peasy */
case REORDER_BUFFER_CHANGE_TRUNCATE:
+ {
+ Oid *relids;
+
+ relids = ReorderBufferGetRelids(rb,
+ change->data.truncate.nrelids);
+ memcpy(relids, data, change->data.truncate.nrelids * sizeof(Oid));
+ change->data.truncate.relids = relids;
+
+ break;
+ }
case REORDER_BUFFER_CHANGE_INTERNAL_SPEC_CONFIRM:
case REORDER_BUFFER_CHANGE_INTERNAL_COMMAND_ID:
case REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID:
diff --git a/src/include/replication/reorderbuffer.h b/src/include/replication/reorderbuffer.h
index 1f52f6bde7..7787edf7b6 100644
--- a/src/include/replication/reorderbuffer.h
+++ b/src/include/replication/reorderbuffer.h
@@ -402,6 +402,9 @@ void ReorderBufferReturnTupleBuf(ReorderBuffer *, ReorderBufferTupleBuf *tuple)
ReorderBufferChange *ReorderBufferGetChange(ReorderBuffer *);
void ReorderBufferReturnChange(ReorderBuffer *, ReorderBufferChange *);
+Oid * ReorderBufferGetRelids(ReorderBuffer *, int nrelids);
+void ReorderBufferReturnRelids(ReorderBuffer *, Oid *relids);
+
void ReorderBufferQueueChange(ReorderBuffer *, TransactionId, XLogRecPtr lsn, ReorderBufferChange *);
void ReorderBufferQueueMessage(ReorderBuffer *, TransactionId, Snapshot snapshot, XLogRecPtr lsn,
bool transactional, const char *prefix,
* Re: memory leak when serializing TRUNCATE in reorderbuffer
2018-06-20 19:42 memory leak when serializing TRUNCATE in reorderbuffer Tomas Vondra <tomas.vondra@2ndquadrant.com>
2018-08-08 19:19 ` Re: memory leak when serializing TRUNCATE in reorderbuffer Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
2018-08-08 19:47 ` Re: memory leak when serializing TRUNCATE in reorderbuffer Tomas Vondra <tomas.vondra@2ndquadrant.com>
2018-08-16 12:24 ` Re: memory leak when serializing TRUNCATE in reorderbuffer Tomas Vondra <tomas.vondra@2ndquadrant.com>
@ 2018-09-03 00:35 ` Tomas Vondra <tomas.vondra@2ndquadrant.com>
0 siblings, 0 replies; 5+ messages in thread
From: Tomas Vondra @ 2018-09-03 00:35 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; pgsql-hackers@lists.postgresql.org
I've pushed this, with some minor tweak, and backpatched to 11 (which is
where TRUNCATE decoding was introduced).
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2018-09-03 00:35 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2018-06-20 19:42 memory leak when serializing TRUNCATE in reorderbuffer Tomas Vondra <tomas.vondra@2ndquadrant.com>
2018-08-08 19:19 ` Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
2018-08-08 19:47 ` Tomas Vondra <tomas.vondra@2ndquadrant.com>
2018-08-16 12:24 ` Tomas Vondra <tomas.vondra@2ndquadrant.com>
2018-09-03 00:35 ` Tomas Vondra <tomas.vondra@2ndquadrant.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox