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

On Wed, Jun 16, 2021 at 9:08 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
>
> Please find attached the latest patch set v86*
>

A couple of comments:

(1)  I think one of my suggested changes was missed (or was that intentional?):

BEFORE:
+                The LSN of the commit prepared.
AFTER:
+                The LSN of the commit prepared transaction.


(2)  In light of Tom Lane's recent changes in:

fe6a20ce54cbbb6fcfe9f6675d563af836ae799a (Don't use Asserts to check
for violations of replication protocol)

there appear to be some instances of such code in these patches.

For example, in the v86-0001 patch:

+/*
+ * Handle PREPARE message.
+ */
+static void
+apply_handle_prepare(StringInfo s)
+{
+ LogicalRepPreparedTxnData prepare_data;
+ char gid[GIDSIZE];
+
+ logicalrep_read_prepare(s, &prepare_data);
+
+ Assert(prepare_data.prepare_lsn == remote_final_lsn);

The above Assert() should be changed to something like:

+    if (prepare_data.prepare_lsn != remote_final_lsn)
+        ereport(ERROR,
+                (errcode(ERRCODE_PROTOCOL_VIOLATION),
+                 errmsg_internal("incorrect prepare LSN %X/%X in
prepare message (expected %X/%X)",
+                                 LSN_FORMAT_ARGS(prepare_data.prepare_lsn),
+                                 LSN_FORMAT_ARGS(remote_final_lsn))));

Without being more familiar with this code, it's difficult for me to
judge exactly how many of such cases are in these patches.

Regards,
Greg Nancarrow
Fujitsu Australia