Re: Skipping logical replication transactions on subscriber side

Hi,

On Fri, Mar 11, 2022 at 8:37 PM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
>
> On Friday, March 11, 2022 5:20 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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

"when we skip transaction" sounds incorrect to me since it's just an
option value but does not indicate that we really skip the transaction
that has that LSN. I realized that we directly use InvalidXLogRecPtr
for subskiplsn so I think no need to mention it.

>
> (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.

I got the comment[1] from Peter to put it at the end, which looks better to me.

>
> (c) parse_subscription_options
>
>
> + /* Parse the argument as LSN */
> + lsn = DatumGetTransactionId(DirectFunctionCall1(pg_lsn_in,
>
>
> Here, shouldn't we call DatumGetLSN, instead of DatumGetTransactionId ?

Right, fixed.

>
>
> (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.

I moved the comment on top of the if statement and removed the brackets.

>
>
> (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

Fixed.

>
>
> * 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.

Fixed.

>
> (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.

Good point, fixed.

On Mon, Mar 14, 2022 at 9:39 PM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
>
> On Friday, March 11, 2022 5:20 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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, few extra comments on v13.
>
>
> (1) src/backend/replication/logical/worker.c
>
>
> With regard to clear_subscription_skip_lsn,
> There are cases that we conduct origin state update twice.
>
> For instance, the case we reset subskiplsn by executing an
> irrelevant non-empty transaction. The first update is
> conducted at apply_handle_commit_internal and the second one
> is at clear_subscription_skip_lsn. In the second change,
> we update replorigin_session_origin_lsn by smaller value(commit_lsn),
> compared to the first update(end_lsn). Were those intentional and OK ?

Good catch, this part is removed in the latest patch.

>
>
> (2) src/backend/replication/logical/worker.c
>
> + * Both origin_lsn and origin_timestamp are the remote transaction's end_lsn
> + * and commit timestamp, respectively.
> + */
> +static void
> +stop_skipping_changes(XLogRecPtr origin_lsn, TimestampTz origin_ts)
>
> Typo. Should change 'origin_timestamp' to 'origin_ts',
> because the name of the argument is the latter.
>
> Also, here we handle not only commit but also prepare.
> You need to fix the comment "commit timestamp" as well.

Fixed.

>
> (3) src/backend/replication/logical/worker.c
>
> +/*
> + * Clear subskiplsn of pg_subscription catalog with origin state update.
> + *
> + * if with_warning is true, we raise a warning when clearing the subskipxid.
>
> It's better to insert this second sentence as the last sentence of
> the other comments.

with_warning is removed in the latest patch.

I've attached an updated version patch.

Regards,

[1] https://www.postgresql.org/message-id/09b80566-c790-704b-35b4-33f87befc41f%40enterprisedb.com

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/