On Friday, March 11, 2022 5:20 PM Masahiko Sawada <sawada.m...@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
