logical streaming of xacts via test_decoding is broken

On Mon, Nov 9, 2020 at 11:00 AM Amit Kapila <amit.kapila16@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.

Additionally, we can extend the existing test case
concurrent_stream.spec to cover this scenario by adding a step to have
an empty transaction before the commit of transaction which we are
going to stream changes for (before s1_commit).

Thoughts?

The analysis seems correct to me, I will work on it.

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

On Mon, Nov 9, 2020 at 11:04 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 11:00 AM Amit Kapila <amit.kapila16@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?

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

On Mon, Nov 9, 2020 at 11:21 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 11:04 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 11:00 AM Amit Kapila <amit.kapila16@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.

--
With Regards,
Amit Kapila.

On Mon, Nov 9, 2020 at 11:31 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Nov 9, 2020 at 11:21 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 11:04 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 11:00 AM Amit Kapila <amit.kapila16@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.

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

On Mon, Nov 9, 2020 at 1:34 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 11:31 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Nov 9, 2020 at 11:21 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 11:04 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 11:00 AM Amit Kapila <amit.kapila16@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?

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

On Mon, Nov 9, 2020 at 3:01 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 1:34 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 11:31 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Nov 9, 2020 at 11:21 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 11:04 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 11:00 AM Amit Kapila <amit.kapila16@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. 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?

--
With Regards,
Amit Kapila.

On Mon, Nov 9, 2020 at 4:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Nov 9, 2020 at 3:01 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 1:34 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 11:31 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Nov 9, 2020 at 11:21 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 11:04 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 11:00 AM Amit Kapila <amit.kapila16@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

On Mon, Nov 9, 2020 at 5:37 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 4:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Nov 9, 2020 at 3:01 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 1:34 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 11:31 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Nov 9, 2020 at 11:21 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 11:04 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 11:00 AM Amit Kapila <amit.kapila16@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.

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

On Mon, Nov 9, 2020 at 6:00 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 5:37 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 4:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

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?

I would prefer option-4 and in addition to that we can ensure that if
skip_empty_xacts = true then by default skip_empty_streams is also
true.

--
With Regards,
Amit Kapila.

On Tue, Nov 10, 2020 at 8:14 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Nov 9, 2020 at 6:00 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 5:37 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 4:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

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?

I would prefer option-4 and in addition to that we can ensure that if
skip_empty_xacts = true then by default skip_empty_streams is also
true.

But then it will behave as a single option only, right? because if
1. skip_empty_xacts = true, then we set skip_empty_streams = true
2. skip_empty_xacts = false then skip_empty_streams can not be set to true

So as per the state machine either both will be true or both will be
false, Am I missing something?

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

On Tue, Nov 10, 2020 at 10:26 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Tue, Nov 10, 2020 at 8:14 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Nov 9, 2020 at 6:00 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 5:37 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 4:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

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?

I would prefer option-4 and in addition to that we can ensure that if
skip_empty_xacts = true then by default skip_empty_streams is also
true.

But then it will behave as a single option only, right? because if
1. skip_empty_xacts = true, then we set skip_empty_streams = true

For this case, users can use skip_empty_xacts = true and
skip_empty_streams = false. I am just asking if the user has only used
skip_empty_xacts = true and didn't use the 'skip_empty_streams'
option.

--
With Regards,
Amit Kapila.

On Tue, Nov 10, 2020 at 10:52 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Nov 10, 2020 at 10:26 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Tue, Nov 10, 2020 at 8:14 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Nov 9, 2020 at 6:00 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 5:37 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 4:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

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?

I would prefer option-4 and in addition to that we can ensure that if
skip_empty_xacts = true then by default skip_empty_streams is also
true.

But then it will behave as a single option only, right? because if
1. skip_empty_xacts = true, then we set skip_empty_streams = true

For this case, users can use skip_empty_xacts = true and
skip_empty_streams = false. I am just asking if the user has only used
skip_empty_xacts = true and didn't use the 'skip_empty_streams'
option.

Ok, thanks for the clarification.

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

On Tue, Nov 10, 2020 at 11:18 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Tue, Nov 10, 2020 at 10:52 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Nov 10, 2020 at 10:26 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Tue, Nov 10, 2020 at 8:14 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Nov 9, 2020 at 6:00 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 5:37 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Nov 9, 2020 at 4:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

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?

I would prefer option-4 and in addition to that we can ensure that if
skip_empty_xacts = true then by default skip_empty_streams is also
true.

But then it will behave as a single option only, right? because if
1. skip_empty_xacts = true, then we set skip_empty_streams = true

For this case, users can use skip_empty_xacts = true and
skip_empty_streams = false. I am just asking if the user has only used
skip_empty_xacts = true and didn't use the 'skip_empty_streams'
option.

Ok, thanks for the clarification.

I have prepared a patch for the same.

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

On Tue, Nov 10, 2020 at 2:25 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Tue, Nov 10, 2020 at 11:18 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Tue, Nov 10, 2020 at 10:52 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

For this case, users can use skip_empty_xacts = true and
skip_empty_streams = false. I am just asking if the user has only used
skip_empty_xacts = true and didn't use the 'skip_empty_streams'
option.

Ok, thanks for the clarification.

I have prepared a patch for the same.

Few comments:
1.
+ else if (strcmp(elem->defname, "skip-empty-streams") == 0)
+ {
+ if (elem->arg == NULL)
+ data->skip_empty_streams = true;
+ else if (!parse_bool(strVal(elem->arg), &data->skip_empty_streams))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("could not parse value \"%s\" for parameter \"%s\"",
+ strVal(elem->arg), elem->defname)));
+ if (!data->skip_empty_xacts && data->skip_empty_streams)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("the skip-empty-streams can not be true if skip-empty-xacts
is false")));
  }

You can probably add a comment as to why we are disallowing this case.
I thought of considering 'stream-changes' parameter here because it
won't make sense to give this parameter without it, however, it seems
that is not necessary but maybe adding a comment
here in that regard would be a good idea.

2.
pg_decode_begin_txn(LogicalDecodingContext *ctx, ReorderBufferTXN *txn)
 {
  TestDecodingData *data = ctx->output_plugin_private;
+ TestDecodingTxnData *txndata =
+ MemoryContextAllocZero(ctx->context, sizeof(TestDecodingTxnData));
+

Shall we free this memory at commit time for the sake of consistency,
otherwise also it would be freed with decoding context?

3. Can you please prepare a separate patch for test case changes so
that it would be easier to verify that it fails without the patch and
passed after the patch?

--
With Regards,
Amit Kapila.

On Tue, Nov 10, 2020 at 7:20 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Nov 10, 2020 at 2:25 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Tue, Nov 10, 2020 at 11:18 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Tue, Nov 10, 2020 at 10:52 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

For this case, users can use skip_empty_xacts = true and
skip_empty_streams = false. I am just asking if the user has only used
skip_empty_xacts = true and didn't use the 'skip_empty_streams'
option.

Ok, thanks for the clarification.

I have prepared a patch for the same.

Few comments:
1.
+ else if (strcmp(elem->defname, "skip-empty-streams") == 0)
+ {
+ if (elem->arg == NULL)
+ data->skip_empty_streams = true;
+ else if (!parse_bool(strVal(elem->arg), &data->skip_empty_streams))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("could not parse value \"%s\" for parameter \"%s\"",
+ strVal(elem->arg), elem->defname)));
+ if (!data->skip_empty_xacts && data->skip_empty_streams)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("the skip-empty-streams can not be true if skip-empty-xacts
is false")));
}

You can probably add a comment as to why we are disallowing this case.
I thought of considering 'stream-changes' parameter here because it
won't make sense to give this parameter without it, however, it seems
that is not necessary but maybe adding a comment
here in that regard would be a good idea.

Should we also consider the case that if the user just passed
skip_empty_streams to true then we should automatically set
skip_empty_xacts to true?
And we will allow the 'skip_empty_streams' parameter only if
stream-changes' is true.

2.
pg_decode_begin_txn(LogicalDecodingContext *ctx, ReorderBufferTXN *txn)
{
TestDecodingData *data = ctx->output_plugin_private;
+ TestDecodingTxnData *txndata =
+ MemoryContextAllocZero(ctx->context, sizeof(TestDecodingTxnData));
+

Shall we free this memory at commit time for the sake of consistency,
otherwise also it would be freed with decoding context?

Yeah, actually I have freed in the stream commit but missed here. I
will do that.

3. Can you please prepare a separate patch for test case changes so
that it would be easier to verify that it fails without the patch and
passed after the patch?

ok

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

On Wed, Nov 11, 2020 at 10:00 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Tue, Nov 10, 2020 at 7:20 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Nov 10, 2020 at 2:25 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

You can probably add a comment as to why we are disallowing this case.
I thought of considering 'stream-changes' parameter here because it
won't make sense to give this parameter without it, however, it seems
that is not necessary but maybe adding a comment
here in that regard would be a good idea.

Should we also consider the case that if the user just passed
skip_empty_streams to true then we should automatically set
skip_empty_xacts to true?

Is there any problem if we don't do this? Actually, adding
dependencies on parameters is confusing so I want to avoid that unless
it is really required.

And we will allow the 'skip_empty_streams' parameter only if
stream-changes' is true.

Can't we simply ignore 'skip_empty_streams' if 'stream-changes' is not given?

--
With Regards,
Amit Kapila.

On Wed, Nov 11, 2020 at 6:59 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Nov 11, 2020 at 10:00 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Tue, Nov 10, 2020 at 7:20 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Nov 10, 2020 at 2:25 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

You can probably add a comment as to why we are disallowing this case.
I thought of considering 'stream-changes' parameter here because it
won't make sense to give this parameter without it, however, it seems
that is not necessary but maybe adding a comment
here in that regard would be a good idea.

Should we also consider the case that if the user just passed
skip_empty_streams to true then we should automatically set
skip_empty_xacts to true?

Is there any problem if we don't do this? Actually, adding
dependencies on parameters is confusing so I want to avoid that unless
it is really required.

And we will allow the 'skip_empty_streams' parameter only if
stream-changes' is true.

The reason behind this thought is that if the user doesn't pass any
value for 'skip_empty_xacts' then the default value will be false and
if the user only pass 'skip_empty_streams' to true then we will error
out assuming that skip_empty_xacts is false but skip_empty_streams is
true. So it seems instead of error out we can assume that
skip_empty_streams true mean skip_empty_xacts is also true if nothing
is passed for that.

Can't we simply ignore 'skip_empty_streams' if 'stream-changes' is not given?

Yeah, we can do that.

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

On Wed, Nov 11, 2020 at 7:05 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Wed, Nov 11, 2020 at 6:59 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Nov 11, 2020 at 10:00 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Tue, Nov 10, 2020 at 7:20 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Nov 10, 2020 at 2:25 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

You can probably add a comment as to why we are disallowing this case.
I thought of considering 'stream-changes' parameter here because it
won't make sense to give this parameter without it, however, it seems
that is not necessary but maybe adding a comment
here in that regard would be a good idea.

Should we also consider the case that if the user just passed
skip_empty_streams to true then we should automatically set
skip_empty_xacts to true?

Is there any problem if we don't do this? Actually, adding
dependencies on parameters is confusing so I want to avoid that unless
it is really required.

And we will allow the 'skip_empty_streams' parameter only if
stream-changes' is true.

The reason behind this thought is that if the user doesn't pass any
value for 'skip_empty_xacts' then the default value will be false and
if the user only pass 'skip_empty_streams' to true then we will error
out assuming that skip_empty_xacts is false but skip_empty_streams is
true. So it seems instead of error out we can assume that
skip_empty_streams true mean skip_empty_xacts is also true if nothing
is passed for that.

So, let's see the overall picture here. We can have four options:
skip_empty_xacts = true, skip_empty_stream = false;
skip_empty_xacts = true, skip_empty_stream = true;
skip_empty_xacts = false, skip_empty_stream = false;
skip_empty_xacts = false, skip_empty_stream = true;

I think we want to say the first three could be supported and for the
last one either we can either give an error or make its behavior
similar to option-2? Is this what your understanding as well?

Another thing I am thinking let's just not expose skip_empty_stream to
the user and consider the behavior based on the value of
skip_empty_xacts. Internally, in the code, we can still have different
variables to distinguish between empty_xacts and empty_streams.

--
With Regards,
Amit Kapila.

On Thu, Nov 12, 2020 at 8:45 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Nov 11, 2020 at 7:05 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Wed, Nov 11, 2020 at 6:59 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Nov 11, 2020 at 10:00 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Tue, Nov 10, 2020 at 7:20 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Nov 10, 2020 at 2:25 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

You can probably add a comment as to why we are disallowing this case.
I thought of considering 'stream-changes' parameter here because it
won't make sense to give this parameter without it, however, it seems
that is not necessary but maybe adding a comment
here in that regard would be a good idea.

Should we also consider the case that if the user just passed
skip_empty_streams to true then we should automatically set
skip_empty_xacts to true?

Is there any problem if we don't do this? Actually, adding
dependencies on parameters is confusing so I want to avoid that unless
it is really required.

And we will allow the 'skip_empty_streams' parameter only if
stream-changes' is true.

The reason behind this thought is that if the user doesn't pass any
value for 'skip_empty_xacts' then the default value will be false and
if the user only pass 'skip_empty_streams' to true then we will error
out assuming that skip_empty_xacts is false but skip_empty_streams is
true. So it seems instead of error out we can assume that
skip_empty_streams true mean skip_empty_xacts is also true if nothing
is passed for that.

So, let's see the overall picture here. We can have four options:
skip_empty_xacts = true, skip_empty_stream = false;
skip_empty_xacts = true, skip_empty_stream = true;
skip_empty_xacts = false, skip_empty_stream = false;
skip_empty_xacts = false, skip_empty_stream = true;

I think we want to say the first three could be supported and for the
last one either we can either give an error or make its behavior
similar to option-2? Is this what your understanding as well?

For the last one if the user has specifically passed false for the
skip_empty_xacts then error and if the user did not pass anything for
skip_empty_xacts then make its behavior similar to option-2.

Another thing I am thinking let's just not expose skip_empty_stream to
the user and consider the behavior based on the value of
skip_empty_xacts. Internally, in the code, we can still have different
variables to distinguish between empty_xacts and empty_streams.

Yeah, even I think in most of the cases it makes more sense to have
skip_empty_xacts and skip_empty_stream similar values. So better we
don't expose skip_empty_stream. I agree that we need to keep two
variables to track the empty stream and empty xacts.

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