Re: Assertion for logically decoding multi inserts into the catalog

> On 6 Aug 2019, at 05:36, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> 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?

Thanks, this is a much better approach and it passes tests for me. +1 on this
version (regardless of outcome of the other patch as this is separate).

cheers ./daniel