On Mon, Nov 9, 2020 at 4:21 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Mon, Nov 9, 2020 at 3:01 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > On Mon, Nov 9, 2020 at 1:34 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > > > On Mon, Nov 9, 2020 at 11:31 AM Amit Kapila <amit.kapil...@gmail.com> > > > wrote: > > > > > > > > On Mon, Nov 9, 2020 at 11:21 AM Dilip Kumar <dilipbal...@gmail.com> > > > > wrote: > > > > > > > > > > On Mon, Nov 9, 2020 at 11:04 AM Dilip Kumar <dilipbal...@gmail.com> > > > > > wrote: > > > > > > > > > > > > On Mon, Nov 9, 2020 at 11:00 AM Amit Kapila > > > > > > <amit.kapil...@gmail.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. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
