To: pgsql-hackers@lists.postgresql.org
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Subject: memory leak when serializing TRUNCATE in reorderbuffer
Message-ID: <66175a41-9342-2845-652f-1bd4c3ee50aa@2ndquadrant.com>
Date: Wed, 20 Jun 2018 21:42:17 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.2
MIME-Version: 1.0
Content-Type: multipart/mixed;
 boundary="------------D1D14DDF9F38C54071BFB59F"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------D1D14DDF9F38C54071BFB59F
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit

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

--------------D1D14DDF9F38C54071BFB59F
Content-Type: text/x-patch;
 name="truncate-spill-fix.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="truncate-spill-fix.patch"

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:

--------------D1D14DDF9F38C54071BFB59F--