RE: Skipping logical replication transactions on subscriber side

On Mon, Jan 17, 2022 2:18 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> 
> I've attached an updated patch. Please review it.
> 

Thanks for updating the patch. Few comments:

1)
        /* Two_phase is only supported in v15 and higher */
         if (pset.sversion >= 150000)
             appendPQExpBuffer(&buf,
-                              ", subtwophasestate AS \"%s\"\n",
-                              gettext_noop("Two phase commit"));
+                              ", subtwophasestate AS \"%s\"\n"
+                              ", subskipxid AS \"%s\"\n",
+                              gettext_noop("Two phase commit"),
+                              gettext_noop("Skip XID"));
 
         appendPQExpBuffer(&buf,
                           ",  subsynccommit AS \"%s\"\n"

I think "skip xid" should be mentioned in the comment. Maybe it could be changed to:
"Two_phase and skip XID are only supported in v15 and higher"

2) The following two places are not consistent in whether "= value" is surround
with square brackets.

+ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> SKIP ( <replaceable
class="parameter">skip_option</replaceable>[= <replaceable class="parameter">value</replaceable>] [, ... ] )
 

+    <term><literal>SKIP ( <replaceable class="parameter">skip_option</replaceable> = <replaceable
class="parameter">value</replaceable>[, ... ] )</literal></term>
 

Should we modify the first place to:
+ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> SKIP ( <replaceable
class="parameter">skip_option</replaceable>= <replaceable class="parameter">value</replaceable> [, ... ] )
 

Because currently there is only one skip_option - xid, and a parameter must be
specified when using it.

3)
+     * Protect subskip_xid of pg_subscription from being concurrently updated
+     * while clearing it.

"subskip_xid" should be "subskipxid" I think.
 
4)
+/*
+ * Start skipping changes of the transaction if the given XID matches the
+ * transaction ID specified by skip_xid option.
+ */

The option name was "skip_xid" in the previous version, and it is "xid" in
latest patch. So should we modify "skip_xid option" to "skip xid option", or
"skip option xid", or something else?

Also the following place has similar issue:
+ * the subscription if hte user has specified skip_xid. Once we start skipping

Regards,
Tang