Обсуждение: Assertion for logically decoding multi inserts into the catalog

My patch for using heap_multi_insert in the catalog [1] failed the logical
decoding part of test/recovery [2].

The assertion it failed on seems to not take multi inserts into the catalog
into consideration, while the main logic does.  This assertion hasn't tripped
since there are no multi inserts into the catalog, but if we introduce them it
will so I’m raising it in a separate thread as it is sort of unrelated from the
patch in question.

The attached patch fixes my test failure and makes sense to me, but this code
is far from my neck of the tree, so I’m really not sure this is the best way to
express the assertion.

cheers ./daniel

[1] https://commitfest.postgresql.org/23/2125/
[2] https://postgr.es/m/CA+hUKGLg1vFiXnkxjp_bea5+VP8D=vHRwSdvj7Rbikr_u4xFbg@mail.gmail.com

On 08/07/2019 22:42, Daniel Gustafsson wrote:
> My patch for using heap_multi_insert in the catalog [1] failed the logical
> decoding part of test/recovery [2].
> 
> The assertion it failed on seems to not take multi inserts into the catalog
> into consideration, while the main logic does.  This assertion hasn't tripped
> since there are no multi inserts into the catalog, but if we introduce them it
> will so I’m raising it in a separate thread as it is sort of unrelated from the
> patch in question.
> 
> The attached patch fixes my test failure and makes sense to me, but this code
> is far from my neck of the tree, so I’m really not sure this is the best way to
> express the assertion.
>
> --- a/src/backend/replication/logical/decode.c
> +++ b/src/backend/replication/logical/decode.c
> @@ -974,7 +974,8 @@ DecodeMultiInsert(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
>          ReorderBufferQueueChange(ctx->reorder, XLogRecGetXid(r),
>                                   buf->origptr, change);
>      }
> -    Assert(data == tupledata + tuplelen);
> +    Assert(xlrec->flags & XLH_INSERT_CONTAINS_NEW_TUPLE &&
> +           data == tupledata + tuplelen);
>  }
>  
>  /*

This patch makes the assertion more strict than it was before. I don't 
see how it could possibly make a regression failure go away. On the 
contrary. So, huh?

- Heikki

> On 31 Jul 2019, at 19:20, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

> This patch makes the assertion more strict than it was before. I don't see how it could possibly make a regression
failurego away. On the contrary. So, huh? 

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.

cheers ./daniel

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

> On 6 Aug 2019, at 05:36, Michael Paquier <michael@paquier.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

On Tue, Aug 06, 2019 at 03:08:48PM +0200, Daniel Gustafsson wrote:
> 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).

I had an extra lookup at this stuff this morning, and applied the
patch.  Please note that I have kept the assertion on tupledata which
cannot be NULL and added a comment about that because it is not
possible to finish yet in a state where we do not have tuple data in
this context, but it actually could be the case if we begin to use
multi-inserts with system catalogs, so the assertion is here to make
future patch authors careful about that.  We could in this case bypass
DecodeMultiInsert() if tupledata is NULL and assert that
XLH_INSERT_CONTAINS_NEW_TUPLE should not be set, or we could just
bypass the logic if XLH_INSERT_CONTAINS_NEW_TUPLE is not set at all.
Let's sort that out in your other patch.
--
Michael