On Thu, Jan 13, 2022 at 10:07 AM tanghy.fnst@fujitsu.com
<tanghy.fnst@fujitsu.com> wrote:
>
> On Wed, Jan 12, 2022 2:02 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > I've attached an updated patch that incorporated all comments I got so far.
> >
>
> Thanks for updating the patch. Here are some comments:
Thank you for the comments!
>
> 1)
> + Skip applying changes of the particular transaction. If incoming data
>
> Should "Skip" be "Skips" ?
>
> 2)
> + prepared by enabling <literal>two_phase</literal> on susbscriber. After h
> + the logical replication successfully skips the transaction, the transaction
>
> The "h" after word "After" seems redundant.
>
> 3)
> + Skipping the whole transaction includes skipping the cahnge that may not violate
>
> "cahnge" should be "changes" I think.
>
> 4)
> +/*
> + * True if we are skipping all data modification changes (INSERT, UPDATE, etc.) of
> + * the specified transaction at MySubscription->skipxid. Once we start skipping
> ...
> + */
> +static TransactionId skipping_xid = InvalidTransactionId;
> +#define is_skipping_changes() (TransactionIdIsValid(skipping_xid))
>
> Maybe we should modify this comment. Something like:
> skipping_xid is valid if we are skipping all data modification changes ...
>
> 5)
> + if (!superuser())
> + ereport(ERROR,
> + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> + errmsg("must be superuser to set %s",
"skip_xid")));
>
> Should we change the message to "must be superuser to skip xid"?
> Because the SQL stmt is "ALTER SUBSCRIPTION ... SKIP (xid = XXX)".
I agree with all the comments above. These are incorporated into the
latest v4 patch I've just submitted[1].
Regards,
[1] postgresql.org/message-id/CAD21AoBZC87nY1pCaexk1uBA68JSBmy2-UqLGirT9g-RVMhjKw%40mail.gmail.com
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/