Re: logical streaming of xacts via test_decoding is broken

On Mon, Nov 9, 2020 at 5:37 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Mon, Nov 9, 2020 at 4:21 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Mon, Nov 9, 2020 at 3:01 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > >
> > > On Mon, Nov 9, 2020 at 1:34 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > > >
> > > > On Mon, Nov 9, 2020 at 11:31 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > > >
> > > > > On Mon, Nov 9, 2020 at 11:21 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > > > > >
> > > > > > On Mon, Nov 9, 2020 at 11:04 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > > > > > >
> > > > > > > On Mon, Nov 9, 2020 at 11:00 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > > > > > >
> > > > > > > > Michael reported a BF failure [1] related to one of the logical
> > > > > > > > streaming test case and I've analyzed the issue. As responded on
> > > > > > > > pgsql-committers [2], the issue here is that the streaming
> > > > > > > > transactions can be interleaved and because we are maintaining whether
> > > > > > > > xact_wrote_changes at the LogicalDecodingContext level, one of later
> > > > > > > > transaction can overwrite the flag for previously streaming
> > > > > > > > transaction. I think it is logical to have this flag at each
> > > > > > > > transaction level (aka in ReorderBufferTxn), however till now it was
> > > > > > > > fine because the changes of each transaction are decoded at one-shot
> > > > > > > > which will be no longer true. We can keep a output_plugin_private data
> > > > > > > > pointer in ReorderBufferTxn which will be used by test_decoding module
> > > > > > > > to keep this and any other such flags in future. We need to set this
> > > > > > > > flag at begin_cb and stream_start_cb APIs and then reset/remove it at
> > > > > > > > stream_commit_cb, stream_abort_cb and stream_stop_cb APIs.
> > > > > >
> > > > > > So IIUC, we need to keep 'output_plugin_private' in
> > > > > > LogicalDecodingContext as well as in ReorderBufferTxn, So the
> > > > > > output_plugin_private in the ReorderBufferTxn will currently just keep
> > > > > > one flag xact_wrote_changes and the remaining things will still be
> > > > > > maintained in output_plugin_private of the LogicalDecodingContext. Is
> > > > > > my understanding correct?
> > > > > >
> > > > >
> > > > > Yes. But keep it as void * so that we can add more things later if required.
> > > >
> > > > Yeah, that makes sense to me.
> > >
> > > I have made some POC changes and analyzed this further, I think that
> > > for the streaming transaction we need 2 flags
> > > 1) xact_wrote_changes 2) stream_wrote_changes
> > >
> > > So basically, if the stream didn't make any changes we can skip the
> > > stream start and stream stop message for the empty stream, but if any
> > > of the streams has made any change then we need to emit the
> > > transaction commit message. But if we want to avoid tracking the
> > > changes per stream then maybe once we set the xact_wrote_changes to
> > > true once for the txn then we better emit the message for all the
> > > stream without tracking whether the stream is empty or not. What is
> > > your thought on this?
> > >
> >
> > I would prefer to have two separate flags to control this behavior
> > because without that it is quite possible that in some of the cases we
> > display empty stream start/stop messages even when that is not
> > intended.
>
> +1
>
> The bigger question is do we want to give users an option
> > for skip_empty_streams similar to skip_empty_xacts? I would again
> > prefer to give a separate option to the user as well. What do you
> > think?
>
> Yeah, I think giving an option would be better.

I think we should also think about the combinations of the
skip_empty_xacts and skip_empty_streams. For example, if the user
passes the skip_empty_xacts to false and skip_empty_streams to true
then what should be the behavior, if the complete transaction
option1: It should not print any stream_start/stream_stop and just
print commit stream because skip_empty_xacts is false and
skip_empty_streams is true.
option2: It should print the stream_start message for the very first
stream because it is the first stream if the txn and skip_empty_xacts
is false so print it and later it will print-stream commit.
option3: Or for the first stream we first put the BEGIN message i.e
stream begin
stream start
stream stop
stream commit
option4: the user should not be allowed to pass skip_empty_xacts =
false with skip_empty_streams to true. Because if the streaming mode
is on then we can not print the xact without printing streams.

What is your opinion on this?

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