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

On Sat, Mar 6, 2021 at 7:19 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Please find attached the latest patch set v50*
>

Few comments on the latest patch series:
=================================
1. I think we can extract the changes to make streaming optional with
2PC and infact you can start a separate thread for it.

2. I think we can get rid of table-sync early exit patch
(v50-0007-Tablesync-early-exit) as we have kept two_phase off from
tablesync worker. I agree that has its own independent value but it is
not required for this patch series.

3. Now, that we are not supporting streaming with two_pc option, do we
really need the first patch
(v50-0001-Refactor-spool-file-logic-in-worker.c)? I suggest to get rid
of the same unless it is really required. If we decide to remove this
patch, then remove the reference to apply_spooled_messages from 0008
patch.

v50-0005-Support-2PC-txn-subscriber-tests
4.
+###############################
+# Test cases involving DDL.
+###############################
+
+# TODO This can be added after we add functionality to replicate DDL
changes to subscriber.

We can remove this from the patch.

v50-0006-Support-2PC-txn-Subscription-option
5.
- /* Binary mode and streaming are only supported in v14 and higher */
+ /* Binary mode and streaming and Two phase commit are only supported
in v14 and higher */

It looks odd that only one of the option starts with capital letter
/Two/two. I suggest to two_phase.

v50-0008-Fix-apply-worker-empty-prepare
6. In 0008, the commit message lines are too long, it is difficult to
read those. Try to keep them 75 char long, this is generally what I
use but you can try something else if you want but not as long as you
have kept in this patch.

7.
+ /*
+ * A Problem:
+ *
..
Let's call this the "empty prepare" problem.
+ *
+ * The following code has a 2-part fix for that scenario.

No need to describe it in terms of problem and fix. You can say
something like: "This can lead to "empty prepare". We avoid this by
...."

--
With Regards,
Amit Kapila.