Re: Bug in logical decoding of in-progress transactions

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.

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