On Tuesday, March 15, 2022 3:13 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> I've attached an updated version patch.
A couple of minor comments on v14.
(1) apply_handle_commit_internal
+ if (is_skipping_changes())
+ {
+ stop_skipping_changes();
+
+ /*
+ * Start a new transaction to clear the subskipxid, if not started
+ * yet. The transaction is committed below.
+ */
+ if (!IsTransactionState())
+ StartTransactionCommand();
+ }
+
I suppose we can move this condition check and stop_skipping_changes() call
to the inside of the block we enter when IsTransactionState() returns true.
As the comment of apply_handle_commit_internal() mentions,
it's the helper function for apply_handle_commit() and
apply_handle_stream_commit().
Then, I couldn't think that both callers don't open
a transaction before the call of apply_handle_commit_internal().
For applying spooled messages, we call begin_replication_step as well.
I can miss something, but timing when we receive COMMIT message
without opening a transaction, would be the case of empty transactions
where the subscription (and its subscription worker) is not interested.
If this is true, currently the patch's code includes
such cases within the range of is_skipping_changes() check.
(2) clear_subscription_skip_lsn's comments.
The comments for this function shouldn't touch
update of origin states, now that we don't update those.
+/*
+ * Clear subskiplsn of pg_subscription catalog with origin state updated.
+ *
This applies to other comments.
+ /*
+ * Update the subskiplsn of the tuple to InvalidXLogRecPtr. If user has
+ * already changed subskiplsn before clearing it we don't update the
+ * catalog and don't advance the replication origin state.
...
+ * .... We can reduce the possibility by
+ * logging a replication origin WAL record to advance the origin LSN
+ * instead but there is no way to advance the origin timestamp and it
+ * doesn't seem to be worth doing anything about it since it's a very rare
+ * case.
+ */
Best Regards,
Takamichi Osumi
