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

[vignesh C]

On Tue, May 25, 2021 at 8:54 AM Ajin Cherian <itsa...@gmail.com> wrote:
>
> On Fri, May 21, 2021 at 6:43 PM Peter Smith <smithpb2...@gmail.com> wrote:
>
> > Fixed in v77-0001, v77-0002
>
> Attaching a new patch-set that rebases the patch, addresses review
> comments from Peter as well as a test failure reported by Tang. I've
> also added some new test case into patch-2 authored by Tang.

Thanks for the updated patch, few comments:
1)  Should "The end LSN of the prepare." be changed to "end LSN of the
prepare transaction."?

--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -7538,6 +7538,13 @@ are available since protocol version 3.
 <varlistentry>
 <term>Int64</term>
 <listitem><para>
+                The end LSN of the prepare.
+</para></listitem>
+</varlistentry>
+<varlistentry>
+
+<term>Int64</term>
+<listitem><para>

2) Should the ";" be "," here?
+++ b/doc/src/sgml/catalogs.sgml
@@ -7639,6 +7639,18 @@ SCRAM-SHA-256$<replaceable>&lt;iteration
count&gt;</replaceable>:<replaceable>&l

      <row>
       <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>subtwophasestate</structfield> <type>char</type>
+      </para>
+      <para>
+       State code:
+       <literal>d</literal> = two_phase mode was not requested, so is disabled;
+       <literal>p</literal> = two_phase mode was requested, but is
pending enablement;
+       <literal>e</literal> = two_phase mode was requested, and is enabled.
+      </para></entry>

3) Should end_lsn be commit_end_lsn?
+       prepare_data->commit_end_lsn = pq_getmsgint64(in);
+       if (prepare_data->commit_end_lsn == InvalidXLogRecPtr)
                elog(ERROR, "end_lsn is not set in commit prepared message");
+       prepare_data->prepare_time = pq_getmsgint64(in);

4) This change is not required

diff --git a/src/include/replication/pgoutput.h
b/src/include/replication/pgoutput.h
index 0dc460f..93c6731 100644
--- a/src/include/replication/pgoutput.h
+++ b/src/include/replication/pgoutput.h
@@ -29,5 +29,4 @@ typedef struct PGOutputData
        bool            messages;
        bool            two_phase;
 } PGOutputData;
-
 #endif                                                 /* PGOUTPUT_H */

5) Will the worker receive commit prepared/rollback prepared as we
have skip logic to skip commit prepared / commit rollback in
pgoutput_rollback_prepared_txn and pgoutput_commit_prepared_txn:

+        * It is possible that we haven't received the prepare because
+        * the transaction did not have any changes relevant to this
+        * subscription and was essentially an empty prepare. In which case,
+        * the walsender is optimized to drop the empty transaction and the
+        * accompanying prepare. Silently ignore if we don't find the prepared
+        * transaction.
         */
-       replorigin_session_origin_lsn = prepare_data.end_lsn;
-       replorigin_session_origin_timestamp = prepare_data.commit_time;
+       if (LookupGXact(gid, prepare_data.prepare_end_lsn,
+                                       prepare_data.prepare_time))
+       {

6) I'm not sure if we could add some tests for skip empty prepare
transactions, if possible add few tests.

7) We could add some debug level log messages for the transaction that
will be skipped.

Regards,
Vignesh