Обсуждение: pg_decode_message vs skip_empty_xacts and xact_wrote_changes

Hi All,
Every pg_decode routine except pg_decode_message  that decodes a
transactional change, has following block
/* output BEGIN if we haven't yet */
if (data->skip_empty_xacts && !txndata->xact_wrote_changes)
{
pg_output_begin(ctx, data, txn, false);
}
txndata->xact_wrote_changes = true;

But pg_decode_message() doesn't call pg_output_begin(). If a WAL
message is the first change in the transaction, it won't have a BEGIN
before it. That looks like a bug. Why is pg_decode_message()
exception?

-- 
Best Wishes,
Ashutosh Bapat

On Mon, Jun 26, 2023 at 3:07 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> Hi All,
> Every pg_decode routine except pg_decode_message  that decodes a
> transactional change, has following block
> /* output BEGIN if we haven't yet */
> if (data->skip_empty_xacts && !txndata->xact_wrote_changes)
> {
> pg_output_begin(ctx, data, txn, false);
> }
> txndata->xact_wrote_changes = true;
>
> But pg_decode_message() doesn't call pg_output_begin(). If a WAL
> message is the first change in the transaction, it won't have a BEGIN
> before it. That looks like a bug. Why is pg_decode_message()
> exception?
>

I can't see a reason why we shouldn't have a similar check for
transactional messages. So, agreed this is a bug.

--
With Regards,
Amit Kapila.

On Mon, 26 Jun 2023 at 15:51, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Jun 26, 2023 at 3:07 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > Hi All,
> > Every pg_decode routine except pg_decode_message  that decodes a
> > transactional change, has following block
> > /* output BEGIN if we haven't yet */
> > if (data->skip_empty_xacts && !txndata->xact_wrote_changes)
> > {
> > pg_output_begin(ctx, data, txn, false);
> > }
> > txndata->xact_wrote_changes = true;
> >
> > But pg_decode_message() doesn't call pg_output_begin(). If a WAL
> > message is the first change in the transaction, it won't have a BEGIN
> > before it. That looks like a bug. Why is pg_decode_message()
> > exception?
> >
>
> I can't see a reason why we shouldn't have a similar check for
> transactional messages. So, agreed this is a bug.

Here is a patch having the fix for the same. I have not added any
tests as the existing tests cover this scenario. The same issue is
present in back branches too.
v1-0001-Call-pg_output_begin-in-pg_decode_message-if-it-i_master.patch
can be applied on master, PG15 and PG14,
v1-0001-Call-pg_output_begin-in-pg_decode_message-if-it-i_PG13.patch
patch can be applied on PG13, PG12 and PG11.
Thoughts?

Regards,
Vignesh

Hi Vignesh,
Thanks for working on this.

On Wed, Jun 28, 2023 at 4:52 PM vignesh C <vignesh21@gmail.com> wrote:
>
> Here is a patch having the fix for the same. I have not added any
> tests as the existing tests cover this scenario. The same issue is
> present in back branches too.

Interesting, we have a test for this scenario and it accepts erroneous
output :).

> v1-0001-Call-pg_output_begin-in-pg_decode_message-if-it-i_master.patch
> can be applied on master, PG15 and PG14,
> v1-0001-Call-pg_output_begin-in-pg_decode_message-if-it-i_PG13.patch
> patch can be applied on PG13, PG12 and PG11.
> Thoughts?

I noticed this when looking at Tomas's patches for logical decoding of
sequences. The code block you have added is repeated in
pg_decode_change() and pg_decode_truncate(). It might be better to
push the conditions in pg_output_begin() itself so that any future
callsite of pg_output_begin() automatically takes care of these
conditions.

Otherwise the patches look good to me.

--
Best Wishes,
Ashutosh Bapat

On Wed, 28 Jun 2023 at 19:26, Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> Hi Vignesh,
> Thanks for working on this.
>
> On Wed, Jun 28, 2023 at 4:52 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Here is a patch having the fix for the same. I have not added any
> > tests as the existing tests cover this scenario. The same issue is
> > present in back branches too.
>
> Interesting, we have a test for this scenario and it accepts erroneous
> output :).
>
> > v1-0001-Call-pg_output_begin-in-pg_decode_message-if-it-i_master.patch
> > can be applied on master, PG15 and PG14,
> > v1-0001-Call-pg_output_begin-in-pg_decode_message-if-it-i_PG13.patch
> > patch can be applied on PG13, PG12 and PG11.
> > Thoughts?
>
> I noticed this when looking at Tomas's patches for logical decoding of
> sequences. The code block you have added is repeated in
> pg_decode_change() and pg_decode_truncate(). It might be better to
> push the conditions in pg_output_begin() itself so that any future
> callsite of pg_output_begin() automatically takes care of these
> conditions.

Thanks for the comments, here is an updated patch handling the above issue.

Regards,
Vignesh

On Thursday, June 29, 2023 12:06 PM vignesh C <vignesh21@gmail.com> wrote:
> 
> On Wed, 28 Jun 2023 at 19:26, Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > Hi Vignesh,
> > Thanks for working on this.
> >
> > On Wed, Jun 28, 2023 at 4:52 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > Here is a patch having the fix for the same. I have not added any
> > > tests as the existing tests cover this scenario. The same issue is
> > > present in back branches too.
> >
> > Interesting, we have a test for this scenario and it accepts erroneous
> > output :).
> >
> > > v1-0001-Call-pg_output_begin-in-pg_decode_message-if-it-i_master.pat
> > > ch can be applied on master, PG15 and PG14,
> > > v1-0001-Call-pg_output_begin-in-pg_decode_message-if-it-i_PG13.patch
> > > patch can be applied on PG13, PG12 and PG11.
> > > Thoughts?
> >
> > I noticed this when looking at Tomas's patches for logical decoding of
> > sequences. The code block you have added is repeated in
> > pg_decode_change() and pg_decode_truncate(). It might be better to
> > push the conditions in pg_output_begin() itself so that any future
> > callsite of pg_output_begin() automatically takes care of these
> > conditions.
> 
> Thanks for the comments, here is an updated patch handling the above issue.

Thanks for the patches.

I tried to understand the following check:

    /*
      * If asked to skip empty transactions, we'll emit BEGIN at the point
      * where the first operation is received for this transaction.
      */
-    if (data->skip_empty_xacts)
+    if (!(last_write ^ data->skip_empty_xacts) || txndata->xact_wrote_changes)
         return;

I might miss something, but would you mind elaborating on why we use "last_write" in this check?

Best Regard,
Hou zj

On Thu, 29 Jun 2023 at 09:58, Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Thursday, June 29, 2023 12:06 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Wed, 28 Jun 2023 at 19:26, Ashutosh Bapat
> > <ashutosh.bapat.oss@gmail.com> wrote:
> > >
> > > Hi Vignesh,
> > > Thanks for working on this.
> > >
> > > On Wed, Jun 28, 2023 at 4:52 PM vignesh C <vignesh21@gmail.com> wrote:
> > > >
> > > > Here is a patch having the fix for the same. I have not added any
> > > > tests as the existing tests cover this scenario. The same issue is
> > > > present in back branches too.
> > >
> > > Interesting, we have a test for this scenario and it accepts erroneous
> > > output :).
> > >
> > > > v1-0001-Call-pg_output_begin-in-pg_decode_message-if-it-i_master.pat
> > > > ch can be applied on master, PG15 and PG14,
> > > > v1-0001-Call-pg_output_begin-in-pg_decode_message-if-it-i_PG13.patch
> > > > patch can be applied on PG13, PG12 and PG11.
> > > > Thoughts?
> > >
> > > I noticed this when looking at Tomas's patches for logical decoding of
> > > sequences. The code block you have added is repeated in
> > > pg_decode_change() and pg_decode_truncate(). It might be better to
> > > push the conditions in pg_output_begin() itself so that any future
> > > callsite of pg_output_begin() automatically takes care of these
> > > conditions.
> >
> > Thanks for the comments, here is an updated patch handling the above issue.
>
> Thanks for the patches.
>
> I tried to understand the following check:
>
>         /*
>          * If asked to skip empty transactions, we'll emit BEGIN at the point
>          * where the first operation is received for this transaction.
>          */
> -       if (data->skip_empty_xacts)
> +       if (!(last_write ^ data->skip_empty_xacts) || txndata->xact_wrote_changes)
>                 return;
>
> I might miss something, but would you mind elaborating on why we use "last_write" in this check?

last_write is used to indicate if it is begin/"begin
prepare"(last_write is true) or change/truncate/message(last_write is
false).

We have specified logical XNOR which will be true for the following conditions:
Condition1: last_write && data->skip_empty_xacts  -> If it is
begin/begin prepare and user has specified skip empty transactions, we
will return from here, so that the begin message can be appended at
the point where the first operation is received for this transaction.
Condition2: !last_write && !data->skip_empty_xacts -> If it is
change/truncate or message and user has not specified skip empty
transactions, we will return from here as we would have appended the
begin earlier itself.
The txndata->xact_w6rote_changes will be set after the first operation
is received for this transaction during which we would have outputted
the begin message, this condition is to skip outputting begin message
if the begin message was already outputted.

Regards,
Vignesh

On Wed, Jun 28, 2023 at 7:26 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> Hi Vignesh,
> Thanks for working on this.
>
> On Wed, Jun 28, 2023 at 4:52 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Here is a patch having the fix for the same. I have not added any
> > tests as the existing tests cover this scenario. The same issue is
> > present in back branches too.
>
> Interesting, we have a test for this scenario and it accepts erroneous
> output :).
>

This made me look at the original commit d6fa44fc which has introduced
this check and it seems this is done primarily to avoid spurious test
failures due to empty transactions. The proposed change won't help
with that. So, I am not sure if it is worth backpatching this change
as proposed. Though, I see the reasons to improve the code in HEAD due
to the following reasons (a) to maintain consistency among
transactional messages/changes (b) we will still emit Begin/Commit
with transactional messages when skip_empty_xacts is '0', see below
test:

SELECT 'init' FROM
pg_create_logical_replication_slot('regression_slot',
'test_decoding');
SELECT 'msg1' FROM pg_logical_emit_message(true, 'test', 'msg1');
SELECT 'msg2' FROM pg_logical_emit_message(false, 'test', 'msg2');

SELECT data FROM pg_logical_slot_peek_changes('regression_slot', NULL,
NULL, 'force-binary', '0', 'skip-empty-xacts', '1');
                            data
------------------------------------------------------------
 message: transactional: 1 prefix: test, sz: 4 content:msg1
 message: transactional: 0 prefix: test, sz: 4 content:msg2
(2 rows)

SELECT data FROM pg_logical_slot_peek_changes('regression_slot', NULL,
NULL, 'force-binary', '0', 'skip-empty-xacts', '0');
                            data
------------------------------------------------------------
 BEGIN 739
 message: transactional: 1 prefix: test, sz: 4 content:msg1
 COMMIT 739
 message: transactional: 0 prefix: test, sz: 4 content:msg2
(4 rows)

Thoughts?

--
With Regards,
Amit Kapila.

On Thu, Jun 29, 2023 at 9:40 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Thu, 29 Jun 2023 at 09:58, Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On Thursday, June 29, 2023 12:06 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> >
> > Thanks for the patches.
> >
> > I tried to understand the following check:
> >
> >         /*
> >          * If asked to skip empty transactions, we'll emit BEGIN at the point
> >          * where the first operation is received for this transaction.
> >          */
> > -       if (data->skip_empty_xacts)
> > +       if (!(last_write ^ data->skip_empty_xacts) || txndata->xact_wrote_changes)
> >                 return;
> >
> > I might miss something, but would you mind elaborating on why we use "last_write" in this check?
>
> last_write is used to indicate if it is begin/"begin
> prepare"(last_write is true) or change/truncate/message(last_write is
> false).
>
> We have specified logical XNOR which will be true for the following conditions:
> Condition1: last_write && data->skip_empty_xacts  -> If it is
> begin/begin prepare and user has specified skip empty transactions, we
> will return from here, so that the begin message can be appended at
> the point where the first operation is received for this transaction.
> Condition2: !last_write && !data->skip_empty_xacts -> If it is
> change/truncate or message and user has not specified skip empty
> transactions, we will return from here as we would have appended the
> begin earlier itself.
> The txndata->xact_w6rote_changes will be set after the first operation
> is received for this transaction during which we would have outputted
> the begin message, this condition is to skip outputting begin message
> if the begin message was already outputted.
>

I feel the use of last_write has reduced the readability of this part
of the code. It may be that we can add comments to make it clear but I
feel your previous version was much easier to understand.

--
With Regards,
Amit Kapila.

On Fri, 30 Jun 2023 at 09:55, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Jun 29, 2023 at 9:40 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Thu, 29 Jun 2023 at 09:58, Zhijie Hou (Fujitsu)
> > <houzj.fnst@fujitsu.com> wrote:
> > >
> > > On Thursday, June 29, 2023 12:06 PM vignesh C <vignesh21@gmail.com> wrote:
> > > >
> > >
> > > Thanks for the patches.
> > >
> > > I tried to understand the following check:
> > >
> > >         /*
> > >          * If asked to skip empty transactions, we'll emit BEGIN at the point
> > >          * where the first operation is received for this transaction.
> > >          */
> > > -       if (data->skip_empty_xacts)
> > > +       if (!(last_write ^ data->skip_empty_xacts) || txndata->xact_wrote_changes)
> > >                 return;
> > >
> > > I might miss something, but would you mind elaborating on why we use "last_write" in this check?
> >
> > last_write is used to indicate if it is begin/"begin
> > prepare"(last_write is true) or change/truncate/message(last_write is
> > false).
> >
> > We have specified logical XNOR which will be true for the following conditions:
> > Condition1: last_write && data->skip_empty_xacts  -> If it is
> > begin/begin prepare and user has specified skip empty transactions, we
> > will return from here, so that the begin message can be appended at
> > the point where the first operation is received for this transaction.
> > Condition2: !last_write && !data->skip_empty_xacts -> If it is
> > change/truncate or message and user has not specified skip empty
> > transactions, we will return from here as we would have appended the
> > begin earlier itself.
> > The txndata->xact_w6rote_changes will be set after the first operation
> > is received for this transaction during which we would have outputted
> > the begin message, this condition is to skip outputting begin message
> > if the begin message was already outputted.
> >
>
> I feel the use of last_write has reduced the readability of this part
> of the code. It may be that we can add comments to make it clear but I
> feel your previous version was much easier to understand.

+1 for the first version patch, I also felt the first version is
easily understandable.

Regards,
Vignesh

On Mon, Jul 3, 2023 at 4:49 PM vignesh C <vignesh21@gmail.com> wrote:
>
> +1 for the first version patch, I also felt the first version is
> easily understandable.
>

Okay, please find the slightly updated version (changed a comment and
commit message). Unless there are more comments, I'll push this in a
day or two.

--
With Regards,
Amit Kapila.

On Wed, Jul 5, 2023 at 2:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Jul 3, 2023 at 4:49 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > +1 for the first version patch, I also felt the first version is
> > easily understandable.
> >
>
> Okay, please find the slightly updated version (changed a comment and
> commit message). Unless there are more comments, I'll push this in a
> day or two.
>

oops, forgot to attach the patch.

--
With Regards,
Amit Kapila.

On Wed, Jul 5, 2023 at 2:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Jul 5, 2023 at 2:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Jul 3, 2023 at 4:49 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > +1 for the first version patch, I also felt the first version is
> > > easily understandable.
> > >
> >
> > Okay, please find the slightly updated version (changed a comment and
> > commit message). Unless there are more comments, I'll push this in a
> > day or two.
> >
>
> oops, forgot to attach the patch.

I still think that we need to do something so that a new call to
pg_output_begin() automatically takes care of the conditions under
which it should be called. Otherwise, we will introduce a similar
problem in some other place in future.

--
Best Wishes,
Ashutosh Bapat

On Wed, Jul 5, 2023 at 7:20 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Wed, Jul 5, 2023 at 2:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Jul 5, 2023 at 2:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Mon, Jul 3, 2023 at 4:49 PM vignesh C <vignesh21@gmail.com> wrote:
> > > >
> > > > +1 for the first version patch, I also felt the first version is
> > > > easily understandable.
> > > >
> > >
> > > Okay, please find the slightly updated version (changed a comment and
> > > commit message). Unless there are more comments, I'll push this in a
> > > day or two.
> > >
> >
> > oops, forgot to attach the patch.
>
> I still think that we need to do something so that a new call to
> pg_output_begin() automatically takes care of the conditions under
> which it should be called. Otherwise, we will introduce a similar
> problem in some other place in future.
>

AFAIU, this problem is because we forget to conditionally call
pg_output_begin() from pg_decode_message() which can happen with or
without moving conditions inside pg_output_begin(). Also, it shouldn't
be done at the expense of complexity. I find the check added by
Vignesh's v2 patch (+ if (!(last_write ^ data->skip_empty_xacts) ||
txndata->xact_wrote_changes)) a bit difficult to understand and more
error-prone. The others like Hou-San also couldn't understand unless
Vignesh gave an explanation. I also thought of avoiding that check.
Basically, IIUC, the check is added because the patch also removed
'data->skip_empty_xacts' check from
pg_decode_begin_txn()/pg_decode_begin_prepare_txn(). Now, if retain
those checks then it is probably okay but again the similar checks are
still split and that doesn't appear to be better than the v1 or v3
patch version. I am not against improving code in this area and
probably we can consider doing it as a separate patch if we have
better ideas instead of combining it with this patch.

--
With Regards,
Amit Kapila.

On Thu, Jul 6, 2023 at 2:06 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Jul 5, 2023 at 7:20 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > On Wed, Jul 5, 2023 at 2:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Wed, Jul 5, 2023 at 2:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > On Mon, Jul 3, 2023 at 4:49 PM vignesh C <vignesh21@gmail.com> wrote:
> > > > >
> > > > > +1 for the first version patch, I also felt the first version is
> > > > > easily understandable.
> > > > >
> > > >
> > > > Okay, please find the slightly updated version (changed a comment and
> > > > commit message). Unless there are more comments, I'll push this in a
> > > > day or two.
> > > >
> > >
> > > oops, forgot to attach the patch.
> >
> > I still think that we need to do something so that a new call to
> > pg_output_begin() automatically takes care of the conditions under
> > which it should be called. Otherwise, we will introduce a similar
> > problem in some other place in future.
> >
>
> AFAIU, this problem is because we forget to conditionally call
> pg_output_begin() from pg_decode_message() which can happen with or
> without moving conditions inside pg_output_begin(). Also, it shouldn't
> be done at the expense of complexity. I find the check added by
> Vignesh's v2 patch (+ if (!(last_write ^ data->skip_empty_xacts) ||
> txndata->xact_wrote_changes)) a bit difficult to understand and more
> error-prone. The others like Hou-San also couldn't understand unless
> Vignesh gave an explanation. I also thought of avoiding that check.
> Basically, IIUC, the check is added because the patch also removed
> 'data->skip_empty_xacts' check from
> pg_decode_begin_txn()/pg_decode_begin_prepare_txn(). Now, if retain
> those checks then it is probably okay but again the similar checks are
> still split and that doesn't appear to be better than the v1 or v3
> patch version. I am not against improving code in this area and
> probably we can consider doing it as a separate patch if we have
> better ideas instead of combining it with this patch.
>

I have pushed this work. But feel free to propose further
improvements, if you have any better ideas.

--
With Regards,
Amit Kapila.

On Tue, Jul 11, 2023 at 5:59 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

> >
>
> I have pushed this work. But feel free to propose further
> improvements, if you have any better ideas.
>

Thanks. We have fixed the problem. So things are better than they
were. I have been busy with something else so couldn't reply.


--
Best Wishes,
Ashutosh Bapat