Re: Bug in logical decoding of in-progress transactions

On Thu, Sep 10, 2020 at 11:53 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:

> On Thu, Sep 10, 2020 at 11:47 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
>
>> On Thu, Sep 10, 2020 at 11:42 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com>
>> wrote:
>> >
>> > On Thu, Sep 10, 2020 at 11:29 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>> wrote:
>> >>
>> >> Hi,
>> >>
>> >> There is a recent build farm failure [1] in one of the test_decoding
>> >> tests as pointed by Tom Lane [2]. The failure report is shown below:
>> >>
>> >> @@ -71,6 +71,8 @@
>> >> data
>> >> ------------------------------------------
>> >> opening a streamed block for transaction
>> >> + closing a streamed block for transaction
>> >> + opening a streamed block for transaction
>> >> streaming change for transaction
>> >> streaming change for transaction
>> >> streaming change for transaction
>> >> @@ -83,7 +85,7 @@
>> >> streaming change for transaction
>> >> closing a streamed block for transaction
>> >> committing streamed transaction
>> >> -(13 rows)
>> >> +(15 rows)
>> >>
>> >> Here, the symptoms are quite similar to what we have fixed in commit
>> >> 82a0ba7707 which is that an extra empty transaction is being decoded
>> >> in the test. It can happen even if have instructed the test to 'skip
>> >> empty xacts' for streaming transactions because the test_decoding
>> >> plugin APIs (related to streaming changes for in-progress xacts) makes
>> >> no effort to skip such empty xacts. It was kept intentionally like
>> >> that under the assumption that we would never try to stream empty
>> >> xacts but on closer inspection of the code, it seems to me that
>> >> assumption was not correct. Basically, we can pick to stream a
>> >> transaction that has change messages for
>> >> REORDER_BUFFER_CHANGE_INTERNAL_SNAPSHOT and we don't send such
>> >> messages to downstream rather they are just to update the internal
>> >> state. So, in this particular failure, it is possible that autovacuum
>> >> transaction has got such a change message added by one of the other
>> >> committed xact and on trying to stream it we get such additional
>> >> messages. The fix is to skip empty xacts when indicated by the user in
>> >> streaming APIs of test_decoding.
>> >>
>> >> Thoughts?
>> >
>> >
>> > Yeah, that's an issue.
>> >
>> >>
>> >>
>> >> [1] -
>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2020-09-09+03%3A42%3A19
>> >> [2] -
>> https://www.postgresql.org/message-id/118303.1599691636%40sss.pgh.pa.us
>> >>
>> >
>> > I have written a test case to reproduce the same. I have also prepared
>> a patch to skip the empty transaction. And after that, the issue has been
>> fixed. But the extra side effect will be that it would skip any empty
>> stream even if the transaction is not empty. As such I don't see any
>> problem with that but this is not what the user has asked for.
>> >
>>
>> Isn't that true for non-streaming xacts as well? Basically
>> skip-empty-xacts option indicates that if there is no change for
>> 'tuple' or 'message', we skip it.
>>
>
> Yeah, that's right.
>

I have removed some comments which are not valid after this patch.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com