RE: Skipping logical replication transactions on subscriber side

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

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