RE: Skipping logical replication transactions on subscriber side

On Wed, Jan 12, 2022 2:02 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> I've attached an updated patch that incorporated all comments I got so far.
>

Thanks for updating the patch. Here are some comments:

1)
+ Skip applying changes of the particular transaction. If incoming data

Should "Skip" be "Skips" ?

2)
+ prepared by enabling <literal>two_phase</literal> on susbscriber. After h
+ the logical replication successfully skips the transaction, the transaction

The "h" after word "After" seems redundant.

3)
+ Skipping the whole transaction includes skipping the cahnge that may not violate

"cahnge" should be "changes" I think.

4)
+/*
+ * True if we are skipping all data modification changes (INSERT, UPDATE, etc.) of
+ * the specified transaction at MySubscription->skipxid. Once we start skipping
...
+ */
+static TransactionId skipping_xid = InvalidTransactionId;
+#define is_skipping_changes() (TransactionIdIsValid(skipping_xid))

Maybe we should modify this comment. Something like:
skipping_xid is valid if we are skipping all data modification changes ...

5)
+ if (!superuser())
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("must be superuser to set %s", "skip_xid")));

Should we change the message to "must be superuser to skip xid"?
Because the SQL stmt is "ALTER SUBSCRIPTION ... SKIP (xid = XXX)".

Regards,
Tang