Re: pg_decode_message vs skip_empty_xacts and xact_wrote_changes

On Wed, Jun 28, 2023 at 7:26 PM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>
> Hi Vignesh,
> Thanks for working on this.
>
> On Wed, Jun 28, 2023 at 4:52 PM vignesh C <vignesh21(at)gmail(dot)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.