Re: Skipping logical replication transactions on subscriber side

On Wed, Aug 4, 2021 at 1:02 PM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Tuesday, August 3, 2021 2:49 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > I've attached new patches that incorporate all comments I got so far.
> > Please review them.
>
> Hi,
>
> I had a few comments for the 0003 patch.

Thanks for reviewing the patch!

>
> 1).
> - This clause alters parameters originally set by
> - <xref linkend="sql-createsubscription"/>. See there for more
> - information. The parameters that can be altered
> - are <literal>slot_name</literal>,
> - <literal>synchronous_commit</literal>,
> - <literal>binary</literal>, and
> - <literal>streaming</literal>.
> + This clause sets or resets a subscription option. The parameters that can be
> + set are the parameters originally set by <xref linkend="sql-createsubscription"/>:
> + <literal>slot_name</literal>, <literal>synchronous_commit</literal>,
> + <literal>binary</literal>, <literal>streaming</literal>.
> + </para>
> + <para>
> + The parameters that can be reset are: <literal>streaming</literal>,
> + <literal>binary</literal>, <literal>synchronous_commit</literal>.
>
> Maybe the doc looks better like the following ?
>
> + This clause alters parameters originally set by
> + <xref linkend="sql-createsubscription"/>. See there for more
> + information. The parameters that can be set
> + are <literal>slot_name</literal>,
> + <literal>synchronous_commit</literal>,
> + <literal>binary</literal>, and
> + <literal>streaming</literal>.
> + </para>
> + <para>
> + The parameters that can be reset are: <literal>streaming</literal>,
> + <literal>binary</literal>, <literal>synchronous_commit</literal>.

Agreed.

>
> 2).
> - opts->create_slot = defGetBoolean(defel);
> + if (!is_reset)
> + opts->create_slot = defGetBoolean(defel);
> }
>
> Since we only support RESET streaming/binary/synchronous_commit, it
> might be unnecessary to add the check 'if (!is_reset)' for other
> option.

Good point.

>
> 3).
> typedef struct AlterSubscriptionStmt
> {
> NodeTag type;
> AlterSubscriptionType kind; /* ALTER_SUBSCRIPTION_OPTIONS, etc */
>
> Since the patch change the remove the enum value
> 'ALTER_SUBSCRIPTION_OPTIONS', it'd better to change the comment here
> as well.

Agreed.

I'll incorporate those comments in the next version patch.

Regards,

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