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
