From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: logical decoding vs. VACUUM FULL / CLUSTER on table with TOAST-ed data
Date: Sat, 24 Nov 2018 00:20:40 +0100
Message-ID: <7a4f2548-d574-c9e4-620f-cae4f00ffb45@2ndquadrant.com> (raw)
In-Reply-To: <9d8e9a45-11e6-740a-b387-83c1c0829263@2ndquadrant.com>
References: <1a17c643-e9af-3dba-486b-fbe31bc1823a@2ndquadrant.com>
<CAD21AoB0EfKjgvo3wsT8qhbUpUR=e-B0iqxcv16A-EQujxp0xg@mail.gmail.com>
<9d8e9a45-11e6-740a-b387-83c1c0829263@2ndquadrant.com>
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
Attachments:rewrite-toast-fix-v2.patch (text/x-patch, 3.7K) downloaddiff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.cindex 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.cindex 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.cindex 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:
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tomas.vondra@2ndquadrant.com, sawada.mshk@gmail.com
Subject: Re: logical decoding vs. VACUUM FULL / CLUSTER on table with TOAST-ed data
In-Reply-To: <7a4f2548-d574-c9e4-620f-cae4f00ffb45@2ndquadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
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