RE: Skipping logical replication transactions on subscriber side

On Mon, Jan 17, 2022 2:18 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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