Re: Skipping logical replication transactions on subscriber side

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

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/