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