Re: Assertion for logically decoding multi inserts into the catalog

[Michael Paquier]

On Tue, Aug 06, 2019 at 12:52:09AM +0200, Daniel Gustafsson wrote:
> Yeah, this is clearly fat-fingered, the intent is to only run the Assert in
> case XLH_INSERT_CONTAINS_NEW_TUPLE is set in xlrec->flags, as it only applies
> under that condition.  The attached is tested in both in the multi-insert 
> patch
> and on HEAD, but I wish I could figure out a better way to express this 
> Assert.

-       Assert(data == tupledata + tuplelen);
+       Assert(data == tupledata + tuplelen ||
+                  ~(xlrec->flags & XLH_INSERT_CONTAINS_NEW_TUPLE));
I find this way to formulate the assertion a bit confusing, as what
you want is basically to make sure that XLH_INSERT_CONTAINS_NEW_TUPLE
is not set in the context of catalogs.  So you could just use that
instead:
(xlrec->flags & XLH_INSERT_CONTAINS_NEW_TUPLE) == 0

Anyway, if you make a parallel with heap_multi_insert() and the way
each xl_multi_insert_tuple is built, I think that the error does not
come from this assertion, but with the way the data length is computed
in DecodeMultiInsert as a move to the next chunk of tuple data is only
done if XLH_INSERT_CONTAINS_NEW_TUPLE is set.  So, in my opinion,
something to fix here is to make sure that we compute the correct
length even if XLH_INSERT_CONTAINS_NEW_TUPLE is *not* set, and then
make sure at the end that the tuple length matches to the end.

This way, we also make sure that we never finish on a state where
the block data associated to the multi-insert record is NULL but
because of a mistake there is some tuple data detected, or that the
tuple data set has a final length which matches the expected outcome.
And actually, it seems to me that this happens in your original patch
to open access to multi-insert for catalogs, because for some reason
XLogRecGetBlockData() returns NULL with a non-zero tuplelen in
DecodeMultiInsert().  I can see that with the TAP test
010_logical_decoding_timelines.pl

Attached is a patch for that.  Thoughts? 
--
Michael

diff --git a/src/backend/replication/logical/decode.c b/src/backend/replication/logical/decode.c
index 151c3ef882..81d2574f9f 100644
--- a/src/backend/replication/logical/decode.c
+++ b/src/backend/replication/logical/decode.c
@@ -901,6 +901,7 @@ DecodeMultiInsert(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
 		return;
 
 	tupledata = XLogRecGetBlockData(r, 0, &tuplelen);
+	Assert(tupledata != NULL);
 
 	data = tupledata;
 	for (i = 0; i < xlrec->ntuples; i++)
@@ -916,6 +917,10 @@ DecodeMultiInsert(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
 
 		memcpy(&change->data.tp.relnode, &rnode, sizeof(RelFileNode));
 
+		xlhdr = (xl_multi_insert_tuple *) SHORTALIGN(data);
+		data = ((char *) xlhdr) + SizeOfMultiInsertTuple;
+		datalen = xlhdr->datalen;
+
 		/*
 		 * CONTAINS_NEW_TUPLE will always be set currently as multi_insert
 		 * isn't used for catalogs, but better be future proof.
@@ -927,10 +932,6 @@ DecodeMultiInsert(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
 		{
 			HeapTupleHeader header;
 
-			xlhdr = (xl_multi_insert_tuple *) SHORTALIGN(data);
-			data = ((char *) xlhdr) + SizeOfMultiInsertTuple;
-			datalen = xlhdr->datalen;
-
 			change->data.tp.newtuple =
 				ReorderBufferGetTupleBuf(ctx->reorder, datalen);
 
@@ -953,8 +954,6 @@ DecodeMultiInsert(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
 			memcpy((char *) tuple->tuple.t_data + SizeofHeapTupleHeader,
 				   (char *) data,
 				   datalen);
-			data += datalen;
-
 			header->t_infomask = xlhdr->t_infomask;
 			header->t_infomask2 = xlhdr->t_infomask2;
 			header->t_hoff = xlhdr->t_hoff;
@@ -973,6 +972,9 @@ DecodeMultiInsert(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
 
 		ReorderBufferQueueChange(ctx->reorder, XLogRecGetXid(r),
 								 buf->origptr, change);
+
+		/* move to the next xl_multi_insert_tuple entry */
+		data += datalen;
 	}
 	Assert(data == tupledata + tuplelen);
 }

signature.asc
Description: PGP signature