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

On Thu, Apr 29, 2021 at 2:23 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Please find attached the latest patch set v74*
>
> Differences from v73* are:
>
> * Rebased to HEAD @ 2 days ago.
>
> * v74 addresses most of the feedback comments from Vignesh posts [1][2][3].
>

Thanks for the updated patch.
Few comments:
1) I felt skey[2] should be skey as we are just using one key here.

+ ScanKeyData skey[2];
+ SysScanDesc scan;
+ bool has_subrels;
+
+ rel = table_open(SubscriptionRelRelationId, AccessShareLock);
+
+ ScanKeyInit(&skey[nkeys++],
+ Anum_pg_subscription_rel_srsubid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(subid));
+
+ scan = systable_beginscan(rel, InvalidOid, false,
+ NULL, nkeys, skey);
+

2) I felt we can change lsn data type from Int64 to XLogRecPtr
+<varlistentry>
+<term>Int64</term>
+<listitem><para>
+ The LSN of the prepare.
+</para></listitem>
+</varlistentry>
+
+<varlistentry>
+<term>Int64</term>
+<listitem><para>
+ The end LSN of the transaction.
+</para></listitem>
+</varlistentry>

3) I felt we can change lsn data type from Int32 to TransactionId
+<varlistentry>
+<term>Int32</term>
+<listitem><para>
+ Xid of the subtransaction (will be same as xid of the
transaction for top-level
+ transactions).
+</para></listitem>
+</varlistentry>

4) Should we change this to "The end LSN of the prepared transaction"
just to avoid any confusion of it meaning commit/rollback.
+<varlistentry>
+<term>Int64</term>
+<listitem><para>
+ The end LSN of the transaction.
+</para></listitem>
+</varlistentry>

Similar problems related to comments 2 and 3 are being discussed at
[1], we can change it accordingly based on the conclusion in the other
thread.
[1] - https://www.postgresql.org/message-id/flat/CAHut%2BPs2JsSd_OpBR9kXt1Rt4bwyXAjh875gUpFw6T210ttO7Q%40mail.gmail.com#cf2a85d0623dcadfbb1204a196681313

Regards,
Vignesh