Re: [HACKERS] logical decoding of two-phase transactions - Mailing list pgsql-hackers

From Ajin Cherian
Subject Re: [HACKERS] logical decoding of two-phase transactions
Date
Msg-id CAFPTHDYnRKDvzgDxoMn_CKqXA-D0MtrbyJvfvjBsO4G=UHDXkg@mail.gmail.com
Whole thread Raw
In response to Re: [HACKERS] logical decoding of two-phase transactions  (Amit Kapila <amit.kapila16@gmail.com>)
Responses Re: [HACKERS] logical decoding of two-phase transactions
List pgsql-hackers
On Sat, Dec 19, 2020 at 2:13 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

> Okay, I have changed the rollback_prepare API as discussed above and
> accordingly handle the case where rollback is received without prepare
> in apply_handle_rollback_prepared.


I have reviewed and tested your new patchset, I agree with all the
changes that you have made and have tested quite a few scenarios and
they seem to be working as expected.
No major comments but some minor observations:

Patch 1:
logical.c: 984
Comment should be "rollback prepared" rather than "abort prepared".

Patch 2:
decode.c: 737: The comments in the header of DecodePrepare seem out of
place, I think here it should describe what the function does rather
than what it does not.
reorderbuffer.c: 2422: It looks like pg_indent has mangled the
comments, the numbering is no longer aligned.

Patch 5:
worker.c: 753: Type: change "dont" to "don't"

Patch 6: logicaldecoding.sgml
logicaldecoding example is no longer correct. This was true prior to
the changes done to replay prepared transactions after a restart. Now
the whole transaction will get decoded again after the commit
prepared.

postgres=# COMMIT PREPARED 'test_prepared1';
COMMIT PREPARED
postgres=# select * from
pg_logical_slot_get_changes('regression_slot', NULL, NULL,
'two-phase-commit', '1');
    lsn    | xid |                    data
-----------+-----+--------------------------------------------
 0/168A060 | 529 | COMMIT PREPARED 'test_prepared1', txid 529
(1 row)

Patch 8:
worker.c: 2798 :
worker.c: 3445 : disabling two-phase in tablesync worker.
 considering new design of multiple commits in tablesync, do we need
to disable two-phase in tablesync?

Other than this I've noticed a few typos that are not in the patch but
in the surrounding code.
logical.c: 1383: Comment should mention stream_commit_cb not stream_abort_cb.
decode.c: 686 - Extra "it's" here:  "because it's it happened"

regards,
Ajin Cherian
Fujitsu Australia



pgsql-hackers by date:

Previous
From: Lukas Meisegeier
Date:
Subject: Re: Feature Proposal: Add ssltermination parameter for SNI-based LoadBalancing
Next
From: Amit Kapila
Date:
Subject: Re: [Patch] Optimize dropping of relation buffers using dlist