Subject: Re: logical decoding vs. VACUUM FULL / CLUSTER on table with TOAST-ed
 data
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
References: <1a17c643-e9af-3dba-486b-fbe31bc1823a@2ndquadrant.com>
 <CAD21AoB0EfKjgvo3wsT8qhbUpUR=e-B0iqxcv16A-EQujxp0xg@mail.gmail.com>
 <9d8e9a45-11e6-740a-b387-83c1c0829263@2ndquadrant.com>
Message-ID: <7a4f2548-d574-c9e4-620f-cae4f00ffb45@2ndquadrant.com>
Date: Sat, 24 Nov 2018 00:20:40 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.3.0
MIME-Version: 1.0
In-Reply-To: <9d8e9a45-11e6-740a-b387-83c1c0829263@2ndquadrant.com>
Content-Type: multipart/mixed;
 boundary="------------CD029630F55B9050B5199257"
Content-Language: en-US
Precedence: bulk

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

On 11/19/18 11:44 AM, Tomas Vondra wrote:
> On 11/19/18 10:28 AM, Masahiko Sawada wrote:
>> On Mon, Nov 19, 2018 at 6:52 AM Tomas Vondra
>> <tomas.vondra@2ndquadrant.com> wrote:
>>>
>>> ...
>>>
>>> This does fix the issue, because we still decode the TOAST changes but
>>> there are no data and so
>>>
>>>      if (change->data.tp.newtuple != NULL)
>>>      {
>>>          dlist_delete(&change->node);
>>>          ReorderBufferToastAppendChunk(rb, txn, relation,
>>>                                        change);
>>>      }
>>>
>>> ends up not stashing the change in the hash table.
>>
>> With the below change you proposed can we remove the above condition
>> because toast-insertion changes being processed by the reorder buffer
>> always have a new tuple? If a toast-insertion record doesn't have a
>> new tuple it has already ignored when decoding.
>>
> 
> Good point. I think you're right the reorderbuffer part may be
> simplified as you propose.
> 

OK, here's an updated patch, tweaking the reorderbuffer part. I plan to
push this sometime mid next week.

regards

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

--------------CD029630F55B9050B5199257
Content-Type: text/x-patch;
 name="rewrite-toast-fix-v2.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="rewrite-toast-fix-v2.patch"

diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index d5bd282f8c..44caeca336 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -659,12 +659,11 @@ raw_heap_insert(RewriteState state, HeapTuple tup)
 			options |= HEAP_INSERT_SKIP_WAL;
 
 		/*
-		 * The new relfilenode's relcache entrye doesn't have the necessary
-		 * information to determine whether a relation should emit data for
-		 * logical decoding.  Force it to off if necessary.
+		 * While rewriting the heap for VACUUM FULL / CLUSTER, make sure data
+		 * for the TOAST table are not logically decoded.  The main heap is
+		 * WAL-logged as XLOG FPI records, which are not logically decoded.
 		 */
-		if (!RelationIsLogicallyLogged(state->rs_old_rel))
-			options |= HEAP_INSERT_NO_LOGICAL;
+		options |= HEAP_INSERT_NO_LOGICAL;
 
 		heaptup = toast_insert_or_update(state->rs_new_rel, tup, NULL,
 										 options);
diff --git a/src/backend/replication/logical/decode.c b/src/backend/replication/logical/decode.c
index afb497227e..e3b05657f8 100644
--- a/src/backend/replication/logical/decode.c
+++ b/src/backend/replication/logical/decode.c
@@ -665,6 +665,9 @@ DecodeAbort(LogicalDecodingContext *ctx, XLogRecordBuffer *buf,
 static void
 DecodeInsert(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
 {
+	Size		datalen;
+	char	   *tupledata;
+	Size		tuplelen;
 	XLogReaderState *r = buf->record;
 	xl_heap_insert *xlrec;
 	ReorderBufferChange *change;
@@ -672,6 +675,13 @@ DecodeInsert(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
 
 	xlrec = (xl_heap_insert *) XLogRecGetData(r);
 
+	/*
+	 * Ignore insert records without new tuples (this does happen when
+	 * raw_heap_insert marks the TOAST record as HEAP_INSERT_NO_LOGICAL).
+	 */
+	if (!(xlrec->flags & XLH_INSERT_CONTAINS_NEW_TUPLE))
+		return;
+
 	/* only interested in our database */
 	XLogRecGetBlockTag(r, 0, &target_node, NULL, NULL);
 	if (target_node.dbNode != ctx->slot->data.database)
@@ -690,17 +700,13 @@ DecodeInsert(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
 
 	memcpy(&change->data.tp.relnode, &target_node, sizeof(RelFileNode));
 
-	if (xlrec->flags & XLH_INSERT_CONTAINS_NEW_TUPLE)
-	{
-		Size		datalen;
-		char	   *tupledata = XLogRecGetBlockData(r, 0, &datalen);
-		Size		tuplelen = datalen - SizeOfHeapHeader;
+	tupledata = XLogRecGetBlockData(r, 0, &datalen);
+	tuplelen = datalen - SizeOfHeapHeader;
 
-		change->data.tp.newtuple =
-			ReorderBufferGetTupleBuf(ctx->reorder, tuplelen);
+	change->data.tp.newtuple =
+		ReorderBufferGetTupleBuf(ctx->reorder, tuplelen);
 
-		DecodeXLogTuple(tupledata, datalen, change->data.tp.newtuple);
-	}
+	DecodeXLogTuple(tupledata, datalen, change->data.tp.newtuple);
 
 	change->data.tp.clear_toast_afterwards = true;
 
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index bed63c768e..23466bade2 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -1598,17 +1598,12 @@ ReorderBufferCommit(ReorderBuffer *rb, TransactionId xid,
 						 * transaction's changes. Otherwise it will get
 						 * freed/reused while restoring spooled data from
 						 * disk.
-						 *
-						 * But skip doing so if there's no tuple-data. That
-						 * happens if a non-mapped system catalog with a toast
-						 * table is rewritten.
 						 */
-						if (change->data.tp.newtuple != NULL)
-						{
-							dlist_delete(&change->node);
-							ReorderBufferToastAppendChunk(rb, txn, relation,
-														  change);
-						}
+						Assert(change->data.tp.newtuple != NULL);
+
+						dlist_delete(&change->node);
+						ReorderBufferToastAppendChunk(rb, txn, relation,
+													  change);
 					}
 
 			change_done:

--------------CD029630F55B9050B5199257--