RE: Skipping logical replication transactions on subscriber side

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 ?

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

(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. It should start with capital letter as well.

Best Regards,
Takamichi Osumi