Re: Skipping logical replication transactions on subscriber side

On Tue, Jan 18, 2022 at 12:04 PM tanghy(dot)fnst(at)fujitsu(dot)com
<tanghy(dot)fnst(at)fujitsu(dot)com> wrote:
>
> 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"

Added.

>
> 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.

Good catch. Fixed.

>
> 3)
> + * Protect subskip_xid of pg_subscription from being concurrently updated
> + * while clearing it.
>
> "subskip_xid" should be "subskipxid" I think.

Fixed.

>
> 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

Fixed.

I've attached an updated patch. All comments I got so far were
incorporated into this patch unless I'm missing something.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/