Re: [HACKERS] logical decoding of two-phase transactions

On Wed, Jun 23, 2021 at 9:10 AM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Tue, Jun 22, 2021 at 3:36 PM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
>
> > Some minor comments:
> >
> > (1)
> > v88-0002
> >
> > doc/src/sgml/logicaldecoding.sgml
> >
> > "examples shows" is not correct.
> > I think there is only ONE example being referred to.
> >
> > BEFORE:
> > + The following examples shows how logical decoding is controlled over the
> > AFTER:
> > + The following example shows how logical decoding is controlled over the
> >
> >
> fixed.
>
> > (2)
> > v88 - 0003
> >
> > doc/src/sgml/ref/create_subscription.sgml
> >
> > (i)
> >
> > BEFORE:
> > + to the subscriber on the PREPARE TRANSACTION. By default,
> > the transaction
> > + prepared on publisher is decoded as a normal transaction at commit.
> > AFTER:
> > + to the subscriber on the PREPARE TRANSACTION. By default,
> > the transaction
> > + prepared on the publisher is decoded as a normal
> > transaction at commit time.
> >
>
> fixed.
>
> > (ii)
> >
> > src/backend/access/transam/twophase.c
> >
> > The double-bracketing is unnecessary:
> >
> > BEFORE:
> > + if ((gxact->valid && strcmp(gxact->gid, gid) == 0))
> > AFTER:
> > + if (gxact->valid && strcmp(gxact->gid, gid) == 0)
> >
>
> fixed.
>
> > (iii)
> >
> > src/backend/replication/logical/snapbuild.c
> >
> > Need to add some commas to make the following easier to read, and
> > change "needs" to "need":
> >
> > BEFORE:
> > + * The prepared transactions that were skipped because previously
> > + * two-phase was not enabled or are not covered by initial snapshot needs
> > + * to be sent later along with commit prepared and they must be before
> > + * this point.
> > AFTER:
> > + * The prepared transactions, that were skipped because previously
> > + * two-phase was not enabled or are not covered by initial snapshot, need
> > + * to be sent later along with commit prepared and they must be before
> > + * this point.
> >
>
> fixed.
>
> > (iv)
> >
> > src/backend/replication/logical/tablesync.c
> >
> > I think the convention used in Postgres code is to check for empty
> > Lists using "== NIL" and non-empty Lists using "!= NIL".
> >
> > BEFORE:
> > + if (table_states_not_ready && !last_start_times)
> > AFTER:
> > + if (table_states_not_ready != NIL && !last_start_times)
> >
> >
> > BEFORE:
> > + else if (!table_states_not_ready && last_start_times)
> > AFTER:
> > + else if (table_states_not_ready == NIL && last_start_times)
> >
>
> fixed.
>
> Also fixed comments from Vignesh:
>
> 1) This content is present in
> v87-0001-Add-option-to-set-two-phase-in-CREATE_REPLICATIO.patch and
> v87-0003-Add-support-for-prepared-transactions-to-built-i.patch, it
> can be removed from one of them
> <varlistentry>
> + <term><literal>TWO_PHASE</literal></term>
> + <listitem>
> + <para>
> + Specify that this logical replication slot supports decoding
> of two-phase
> + transactions. With this option, two-phase commands like
> + <literal>PREPARE TRANSACTION</literal>, <literal>COMMIT
> PREPARED</literal>
> + and <literal>ROLLBACK PREPARED</literal> are decoded and transmitted.
> + The transaction will be decoded and transmitted at
> + <literal>PREPARE TRANSACTION</literal> time.
> + </para>
> + </listitem>
> + </varlistentry>
> +
> + <varlistentry>
>
> I don't see this duplicate content.

Thanks for the updated patch.
The patch v89-0001-Add-option-to-set-two-phase-in-CREATE_REPLICATIO.patch
has the following:
+ <term><literal>TWO_PHASE</literal></term>
+ <listitem>
+ <para>
+ Specify that this logical replication slot supports decoding
of two-phase
+ transactions. With this option, two-phase commands like
+ <literal>PREPARE TRANSACTION</literal>, <literal>COMMIT
PREPARED</literal>
+ and <literal>ROLLBACK PREPARED</literal> are decoded and transmitted.
+ The transaction will be decoded and transmitted at
+ <literal>PREPARE TRANSACTION</literal> time.
+ </para>
+ </listitem>
+ </varlistentry>

The patch v89-0003-Add-support-for-prepared-transactions-to-built-i.patch
has the following:
+ <term><literal>TWO_PHASE</literal></term>
+ <listitem>
+ <para>
+ Specify that this replication slot supports decode of two-phase
+ transactions. With this option, two-phase commands like
+ <literal>PREPARE TRANSACTION</literal>, <literal>COMMIT
PREPARED</literal>
+ and <literal>ROLLBACK PREPARED</literal> are decoded and transmitted.
+ The transaction will be decoded and transmitted at
+ <literal>PREPARE TRANSACTION</literal> time.
+ </para>
+ </listitem>
+ </varlistentry>

We can remove one of them.

Regards,
Vignesh