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
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:
