Re: Skipping logical replication transactions on subscriber side

On Mon, Jan 17, 2022 at 5:18 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> I've attached an updated patch. Please review it.
>

Some review comments for the v6 patch:


doc/src/sgml/logical-replication.sgml

(1) Expanded output

Since the view output is shown in "expanded output" mode, perhaps the
doc should say that, or alternatively add the following lines prior to
it, to make it clear:

  postgres=# \x
  Expanded display is on.


(2) Message output in server log

The actual CONTEXT text now just says "at ..." instead of "with commit
timestamp ...", so the doc needs to be updated as follows:

BEFORE:
+CONTEXT:  processing remote data during "INSERT" for replication
target relation "public.test" in transaction 716 with commit timestamp
2021-09-29 15:52:45.165754+00
AFTER:
+CONTEXT:  processing remote data during "INSERT" for replication
target relation "public.test" in transaction 716 at 2021-09-29
15:52:45.165754+00

(3)
The wording "the change" doesn't seem right here, so I suggest the
following update:

BEFORE:
+   Skipping the whole transaction includes skipping the change that
may not violate
AFTER:
+   Skipping the whole transaction includes skipping changes that may
not violate


doc/src/sgml/ref/alter_subscription.sgml

(4)
I have a number of suggested wording improvements:

BEFORE:
+      Skips applying changes of the particular transaction.  If incoming data
+      violates any constraints the logical replication will stop until it is
+      resolved.  The resolution can be done either by changing data on the
+      subscriber so that it doesn't conflict with incoming change or
by skipping
+      the whole transaction.  The logical replication worker skips all data
+      modification changes within the specified transaction including
the changes
+      that may not violate the constraint, so, it should only be used as a last
+      resort. This option has no effect on the transaction that is already
+      prepared by enabling <literal>two_phase</literal> on subscriber.

AFTER:
+      Skips applying all changes of the specified transaction.  If
incoming data
+      violates any constraints, logical replication will stop until it is
+      resolved.  The resolution can be done either by changing data on the
+      subscriber so that it doesn't conflict with incoming change or
by skipping
+      the whole transaction.  Using the SKIP option, the logical
replication worker skips all data
+      modification changes within the specified transaction, including changes
+      that may not violate the constraint, so, it should only be used as a last
+      resort. This option has no effect on transactions that are already
+      prepared by enabling <literal>two_phase</literal> on the subscriber.


(5)
change -> changes

BEFORE:
+      subscriber so that it doesn't conflict with incoming change or
by skipping
AFTER:
+      subscriber so that it doesn't conflict with incoming changes or
by skipping


src/backend/replication/logical/worker.c

(6) Missing word?
The following should say "worth doing" or "worth it"?

+ * doesn't seem to be worth since it's a very minor case.


src/test/regress/sql/subscription.sql

(7) Misleading test case
I think the following test case is misleading and should be removed,
because the "1.1" xid value is only regarded as invalid because "1" is
an invalid xid (and there's already a test case for a "1" xid) - the
fractional part gets thrown away, and doesn't affect the validity
here.

   +ALTER SUBSCRIPTION regress_testsub SKIP (xid = 1.1);


Regards,
Greg Nancarrow
Fujitsu Australia