RE: Skipping logical replication transactions on subscriber side

On Friday, March 11, 2022 5:20 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> I've attached an updated version patch. This patch can be applied on top of the
> latest disable_on_error patch[1].
Hi, thank you for the patch. I'll share my review comments on v13.


(a) src/backend/commands/subscriptioncmds.c

@@ -84,6 +86,8 @@ typedef struct SubOpts
        bool            streaming;
        bool            twophase;
        bool            disableonerr;
+       XLogRecPtr      lsn;                    /* InvalidXLogRecPtr for resetting purpose,
+                                                                * otherwise a valid LSN */


I think this explanation is slightly odd and can be improved.
Strictly speaking, I feel a *valid* LSN is for retting transaction purpose
from the functional perspective. Also, the wording "resetting purpose"
is unclear by itself. I'll suggest below change.

From:
InvalidXLogRecPtr for resetting purpose, otherwise a valid LSN
To:
A valid LSN when we skip transaction, otherwise InvalidXLogRecPtr

(b) The code position of additional append in describeSubscriptions


+
+               /* Skip LSN is only supported in v15 and higher */
+               if (pset.sversion >= 150000)
+                       appendPQExpBuffer(&buf,
+                                                         ", subskiplsn AS \"%s\"\n",
+                                                         gettext_noop("Skip LSN"));

I suggest to combine this code after subdisableonerr.

(c) parse_subscription_options


+                               /* Parse the argument as LSN */
+                               lsn = DatumGetTransactionId(DirectFunctionCall1(pg_lsn_in,


Here, shouldn't we call DatumGetLSN, instead of DatumGetTransactionId ?


(d) parse_subscription_options

+                       if (strcmp(lsn_str, "none") == 0)
+                       {
+                               /* Setting lsn = NONE is treated as resetting LSN */
+                               lsn = InvalidXLogRecPtr;
+                       }
+

We should remove this pair of curly brackets that is for one sentence.


(e) src/backend/replication/logical/worker.c

+ * to skip applying the changes when starting to apply changes.  The subskiplsn is
+ * cleared after successfully skipping the transaction or applying non-empty
+ * transaction, where the later avoids the mistakenly specified subskiplsn from
+ * being left.

typo "the later" -> "the latter"

At the same time, I feel the last part of this sentence can be an independent sentence.
From:
, where the later avoids the mistakenly specified subskiplsn from being left
To:
. The latter prevents the mistakenly specified subskiplsn from being left


* Note that my comments below are applied if we choose we don't merge disable_on_error test with skip lsn tests.

(f) src/test/subscription/t/030_skip_xact.pl

+use Test::More tests => 4;

It's better to utilize the new style for the TAP test.
Then, probably we should introduce done_testing()
at the end of the test.

(g) src/test/subscription/t/030_skip_xact.pl

I think there's no need to create two types of subscriptions.
Just one subscription with two_phase = on and streaming = on
would be sufficient for the tests(normal commit, commit prepared,
stream commit cases). I think this point of view will reduce
the number of the table and the publication, which will
make the whole test simpler.


Best Regards,
	Takamichi Osumi