Re: pg_decode_message vs skip_empty_xacts and xact_wrote_changes
On Tue, Jul 11, 2023 at 5:59 PM Amit Kapila 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
Re: pg_decode_message vs skip_empty_xacts and xact_wrote_changes
On Thu, Jul 6, 2023 at 2:06 PM Amit Kapila wrote: > > On Wed, Jul 5, 2023 at 7:20 PM Ashutosh Bapat > wrote: > > > > On Wed, Jul 5, 2023 at 2:29 PM Amit Kapila wrote: > > > > > > On Wed, Jul 5, 2023 at 2:28 PM Amit Kapila > > > wrote: > > > > > > > > On Mon, Jul 3, 2023 at 4:49 PM vignesh C 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.
Re: pg_decode_message vs skip_empty_xacts and xact_wrote_changes
On Wed, Jul 5, 2023 at 7:20 PM Ashutosh Bapat wrote: > > On Wed, Jul 5, 2023 at 2:29 PM Amit Kapila wrote: > > > > On Wed, Jul 5, 2023 at 2:28 PM Amit Kapila wrote: > > > > > > On Mon, Jul 3, 2023 at 4:49 PM vignesh C 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.
Re: pg_decode_message vs skip_empty_xacts and xact_wrote_changes
On Wed, Jul 5, 2023 at 2:29 PM Amit Kapila wrote: > > On Wed, Jul 5, 2023 at 2:28 PM Amit Kapila wrote: > > > > On Mon, Jul 3, 2023 at 4:49 PM vignesh C 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
Re: pg_decode_message vs skip_empty_xacts and xact_wrote_changes
On Wed, Jul 5, 2023 at 2:28 PM Amit Kapila wrote: > > On Mon, Jul 3, 2023 at 4:49 PM vignesh C 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. v3-0001-Add-BEGIN-COMMIT-for-transactional-messages-durin.patch Description: Binary data
Re: pg_decode_message vs skip_empty_xacts and xact_wrote_changes
On Mon, Jul 3, 2023 at 4:49 PM vignesh C 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.
Re: pg_decode_message vs skip_empty_xacts and xact_wrote_changes
On Fri, 30 Jun 2023 at 09:55, Amit Kapila wrote: > > On Thu, Jun 29, 2023 at 9:40 PM vignesh C wrote: > > > > On Thu, 29 Jun 2023 at 09:58, Zhijie Hou (Fujitsu) > > wrote: > > > > > > On Thursday, June 29, 2023 12:06 PM vignesh C 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
Re: pg_decode_message vs skip_empty_xacts and xact_wrote_changes
On Thu, Jun 29, 2023 at 9:40 PM vignesh C wrote: > > On Thu, 29 Jun 2023 at 09:58, Zhijie Hou (Fujitsu) > wrote: > > > > On Thursday, June 29, 2023 12:06 PM vignesh C 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.
Re: pg_decode_message vs skip_empty_xacts and xact_wrote_changes
On Wed, Jun 28, 2023 at 7:26 PM Ashutosh Bapat
wrote:
>
> Hi Vignesh,
> Thanks for working on this.
>
> On Wed, Jun 28, 2023 at 4:52 PM vignesh C 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.
Re: pg_decode_message vs skip_empty_xacts and xact_wrote_changes
On Thu, 29 Jun 2023 at 09:58, Zhijie Hou (Fujitsu) wrote: > > On Thursday, June 29, 2023 12:06 PM vignesh C wrote: > > > > On Wed, 28 Jun 2023 at 19:26, Ashutosh Bapat > > wrote: > > > > > > Hi Vignesh, > > > Thanks for working on this. > > > > > > On Wed, Jun 28, 2023 at 4:52 PM vignesh C 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
RE: pg_decode_message vs skip_empty_xacts and xact_wrote_changes
On Thursday, June 29, 2023 12:06 PM vignesh C wrote: > > On Wed, 28 Jun 2023 at 19:26, Ashutosh Bapat > wrote: > > > > Hi Vignesh, > > Thanks for working on this. > > > > On Wed, Jun 28, 2023 at 4:52 PM vignesh C 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
Re: pg_decode_message vs skip_empty_xacts and xact_wrote_changes
On Wed, 28 Jun 2023 at 19:26, Ashutosh Bapat
wrote:
>
> Hi Vignesh,
> Thanks for working on this.
>
> On Wed, Jun 28, 2023 at 4:52 PM vignesh C 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
From 130715a6dd808ed88af8c894d7e27b8637bc2c3a Mon Sep 17 00:00:00 2001
From: Vignesh C
Date: Wed, 28 Jun 2023 14:01:22 +0530
Subject: [PATCH v2] Call pg_output_begin in pg_decode_message if it is the
first change in the transaction.
Call pg_output_begin in pg_decode_message if it is the first change in
the transaction.
---
contrib/test_decoding/expected/messages.out | 10 +--
contrib/test_decoding/sql/messages.sql | 2 +-
contrib/test_decoding/test_decoding.c | 31 +
3 files changed, 29 insertions(+), 14 deletions(-)
diff --git a/contrib/test_decoding/expected/messages.out b/contrib/test_decoding/expected/messages.out
index c75d40190b..0fd70036bd 100644
--- a/contrib/test_decoding/expected/messages.out
+++ b/contrib/test_decoding/expected/messages.out
@@ -58,17 +58,23 @@ SELECT 'ignorethis' FROM pg_logical_emit_message(true, 'test', 'czechtastic');
ignorethis
(1 row)
-SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'force-binary', '0', 'skip-empty-xacts', '1');
+SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'force-binary', '0', 'skip-empty-xacts', '1', 'include-xids', '0');
data
+ BEGIN
message: transactional: 1 prefix: test, sz: 4 content:msg1
+ COMMIT
message: transactional: 0 prefix: test, sz: 4 content:msg2
message: transactional: 0 prefix: test, sz: 4 content:msg4
message: transactional: 0 prefix: test, sz: 4 content:msg6
+ BEGIN
message: transactional: 1 prefix: test, sz: 4 content:msg5
message: transactional: 1 prefix: test, sz: 4 content:msg7
+ COMMIT
+ BEGIN
message: transactional: 1 prefix: test, sz: 11 content:czechtastic
-(7 rows)
+ COMMIT
+(13 rows)
-- test db filtering
\set prevdb :DBNAME
diff --git a/contrib/test_decoding/sql/messages.sql b/contrib/test_decoding/sql/messages.sql
index cf3f7738e5..3d8500f99c 100644
--- a/contrib/test_decoding/sql/messages.sql
+++ b/contrib/test_decoding/sql/messages.sql
@@ -19,7 +19,7 @@ COMMIT;
SELECT 'ignorethis' FROM pg_logical_emit_message(true, 'test', 'czechtastic');
-SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'force-binary', '0', 'skip-empty-xacts', '1');
+SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'force-binary', '0', 'skip-empty-xacts', '1', 'include-xids', '0');
-- test db filtering
\set prevdb :DBNAME
diff --git a/contrib/test_decoding/test_decoding.c b/contrib/test_decoding/test_decoding.c
index 93c948856e..86c29de40b 100644
--- a/contrib/test_decoding/test_decoding.c
+++ b/contrib/test_decoding/test_decoding.c
@@ -211,15 +211,21 @@ pg_decode_begin_txn(LogicalDecodingContext *ctx, ReorderBufferTXN *txn)
TestDecodingData *data = ctx->output_plugin_private;
data->xact_wrote_changes = false;
- if (data->skip_empty_xacts)
- return;
pg_output_begin(ctx, data, txn, true);
}
static void
-pg_output_begin(LogicalDecodingContext *ctx, TestDecodingData *data, ReorderBufferTXN *txn, bool last_write)
+pg_output_begin(LogicalDecodingContext *ctx, TestDecodingData *data,
+ReorderBufferTXN *txn, bool last_write)
{
+ /*
+ * If asked to skip empty transactions, we'll emit BEGIN at the point
+ * where the first operation is received for this transaction.
+ */
+ if (!(last_write ^ data->skip_empty_xacts) || data->xact_wrote_changes)
+ return;
+
OutputPluginPrepareWrite(ctx, last_write);
if (data->include_xids)
appendStringInfo(ctx->out, "BEGIN %u", txn->xid);
@@ -403,10 +409,7 @@ pg_decode_change(LogicalDecodingContext *ctx, ReorderBufferTXN *txn,
data = ctx->output_plugin_private;
/* output BEGIN if we haven't yet */
- if (data->skip_empty_xacts && !data->xact_wrote_changes)
- {
-
Re: pg_decode_message vs skip_empty_xacts and xact_wrote_changes
Hi Vignesh, Thanks for working on this. On Wed, Jun 28, 2023 at 4:52 PM vignesh C 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
Re: pg_decode_message vs skip_empty_xacts and xact_wrote_changes
On Mon, 26 Jun 2023 at 15:51, Amit Kapila wrote:
>
> On Mon, Jun 26, 2023 at 3:07 PM Ashutosh Bapat
> 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
From 2df5e87ec7c82daa5f17da50f770f923eb7765b4 Mon Sep 17 00:00:00 2001
From: Vignesh C
Date: Wed, 28 Jun 2023 14:01:22 +0530
Subject: [PATCH] Call pg_output_begin in pg_decode_message if it is the first
change in the transaction.
Call pg_output_begin in pg_decode_message if it is the first change in
the transaction.
---
contrib/test_decoding/expected/messages.out | 10 --
contrib/test_decoding/sql/messages.sql | 2 +-
contrib/test_decoding/test_decoding.c | 12
3 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/contrib/test_decoding/expected/messages.out b/contrib/test_decoding/expected/messages.out
index c75d40190b..0fd70036bd 100644
--- a/contrib/test_decoding/expected/messages.out
+++ b/contrib/test_decoding/expected/messages.out
@@ -58,17 +58,23 @@ SELECT 'ignorethis' FROM pg_logical_emit_message(true, 'test', 'czechtastic');
ignorethis
(1 row)
-SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'force-binary', '0', 'skip-empty-xacts', '1');
+SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'force-binary', '0', 'skip-empty-xacts', '1', 'include-xids', '0');
data
+ BEGIN
message: transactional: 1 prefix: test, sz: 4 content:msg1
+ COMMIT
message: transactional: 0 prefix: test, sz: 4 content:msg2
message: transactional: 0 prefix: test, sz: 4 content:msg4
message: transactional: 0 prefix: test, sz: 4 content:msg6
+ BEGIN
message: transactional: 1 prefix: test, sz: 4 content:msg5
message: transactional: 1 prefix: test, sz: 4 content:msg7
+ COMMIT
+ BEGIN
message: transactional: 1 prefix: test, sz: 11 content:czechtastic
-(7 rows)
+ COMMIT
+(13 rows)
-- test db filtering
\set prevdb :DBNAME
diff --git a/contrib/test_decoding/sql/messages.sql b/contrib/test_decoding/sql/messages.sql
index cf3f7738e5..3d8500f99c 100644
--- a/contrib/test_decoding/sql/messages.sql
+++ b/contrib/test_decoding/sql/messages.sql
@@ -19,7 +19,7 @@ COMMIT;
SELECT 'ignorethis' FROM pg_logical_emit_message(true, 'test', 'czechtastic');
-SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'force-binary', '0', 'skip-empty-xacts', '1');
+SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'force-binary', '0', 'skip-empty-xacts', '1', 'include-xids', '0');
-- test db filtering
\set prevdb :DBNAME
diff --git a/contrib/test_decoding/test_decoding.c b/contrib/test_decoding/test_decoding.c
index 12d1d0505d..2bbac5f6a8 100644
--- a/contrib/test_decoding/test_decoding.c
+++ b/contrib/test_decoding/test_decoding.c
@@ -743,6 +743,18 @@ pg_decode_message(LogicalDecodingContext *ctx,
ReorderBufferTXN *txn, XLogRecPtr lsn, bool transactional,
const char *prefix, Size sz, const char *message)
{
+ TestDecodingData *data = ctx->output_plugin_private;
+ TestDecodingTxnData *txndata;
+
+ txndata = transactional ? txn->output_plugin_private : NULL;
+
+ /* output BEGIN if we haven't yet */
+ if (transactional && data->skip_empty_xacts && !txndata->xact_wrote_changes)
+ pg_output_begin(ctx, data, txn, false);
+
+ if (transactional)
+ txndata->xact_wrote_changes = true;
+
OutputPluginPrepareWrite(ctx, true);
appendStringInfo(ctx->out, "message: transactional: %d prefix: %s, sz: %zu content:",
transactional, prefix, sz);
--
2.34.1
From 273776c53da22ecd69f8b7d4e7712ef66ab0ea5c Mon Sep 17 00:00:00 2001
From: Vignesh C
Date: Wed, 28 Jun 2023 14:01:22 +0530
Subject: [PATCH] Call pg_output_begin in pg_decode_message if it is the first
change in the transaction.
Call pg_output_begin in pg_decode_message if it is the first change in
Re: pg_decode_message vs skip_empty_xacts and xact_wrote_changes
On Mon, Jun 26, 2023 at 3:07 PM Ashutosh Bapat
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.
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
