On Wed, Jun 23, 2021 at 9:10 AM Ajin Cherian <itsa...@gmail.com> wrote: > > On Tue, Jun 22, 2021 at 3:36 PM Greg Nancarrow <gregn4...@gmail.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