RE: Skipping logical replication transactions on subscriber side

On Fri, Mar 11, 2022 4: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].
> 

Thanks for your patch. Here are some comments for the v13 patch.

1. doc/src/sgml/ref/alter_subscription.sgml
+          Specifies the transaction's finish LSN of the remote transaction whose changes

Could it be simplified to "Specifies the finish LSN of the remote transaction
whose ...".

2.
I met a failed assertion, the backtrace is attached. This is caused by the
following code in maybe_start_skipping_changes().

+        /*
+         * It's a rare case; a past subskiplsn was left because the server
+         * crashed after preparing the transaction and before clearing the
+         * subskiplsn. We clear it without a warning message so as not confuse
+         * the user.
+         */
+        if (unlikely(MySubscription->skiplsn < lsn))
+        {
+            clear_subscription_skip_lsn(MySubscription->skiplsn, InvalidXLogRecPtr, 0,
+                                        false);
+            Assert(!IsTransactionState());
+        }

We want to clear subskiplsn in the case mentioned in comment. But if the next
transaction is a steaming transaction and this function is called by
apply_spooled_messages(), we are inside a transaction here. So, I think this
assertion is not suitable for streaming transaction. Thoughts?

3.
+    XLogRecPtr    subskiplsn;        /* All changes which committed at this LSN are
+                                 * skipped */

To be consistent, should the comment be changed to "All changes which finished
at this LSN are skipped"?

4.
+      After logical replication worker successfully skips the transaction or commits
+      non-empty transaction, the LSN (stored in
+      <structname>pg_subscription</structname>.<structfield>subskiplsn</structfield>)
+      is cleared.

Besides "commits non-empty transaction", subskiplsn would also be cleared in
some two-phase commit cases I think. Like prepare/commit/rollback a transaction,
even if it is an empty transaction. So, should we change it for these cases?

5.
+ * Clear subskiplsn of pg_subscription catalog with origin state update.

Should "with origin state update" modified to "with origin state updated"?

Regards,
Shi yu