Re: Assertion for logically decoding multi inserts into the catalog

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