Re: Skipping logical replication transactions on subscriber side

On Mon, Jul 26, 2021 at 11:58 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Thu, Jul 22, 2021 at 8:53 PM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On July 20, 2021 9:26 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > On Mon, Jul 19, 2021 at 8:38 PM houzj.fnst@fujitsu.com
> > > <houzj.fnst@fujitsu.com> wrote:
> > > >
> > > > On July 19, 2021 2:40 PM Masahiko Sawada <sawada.mshk@gmail.com>
> > > wrote:
> > > > > I've attached the updated version patch that incorporated all
> > > > > comments I got so far except for the clearing error details part I
> > > > > mentioned above. After getting a consensus on those parts, I'll
> > > > > incorporate the idea into the patches.
> > > >
> > > > 3) For 0003 patch, if user set skip_xid to a wrong xid which have not been
> > > >    assigned, and then will the change be skipped when the xid is assigned in
> > > >    the future even if it doesn't cause any conflicts ?
> > >
> > > Yes. Currently, setting a correct xid is the user's responsibility. I think it would
> > > be better to disable it or emit WARNING/ERROR when the user mistakenly set
> > > the wrong xid if we find out a convenient way to detect that.
> >
> > Thanks for the explanation. As Amit suggested, it seems we can document the
> > risk of misusing skip_xid. Besides, I found some minor things in the patch.
> >
> > 1) In 0002 patch
> >
> > + */
> > +static void
> > +pgstat_recv_subscription_purge(PgStat_MsgSubscriptionPurge *msg, int len)
> > +{
> > +       if (subscriptionErrHash != NULL)
> > +               return;
> > +
> >
> > +static void
> > +pgstat_recv_subscription_error(PgStat_MsgSubscriptionErr *msg, int len)
> > +{
> >
> > the second paramater "len" seems not used in the function
> > pgstat_recv_subscription_purge() and pgstat_recv_subscription_error().
> >
>
> 'len' is not used at all in not only functions the patch added but
> also other pgstat_recv_* functions. Can we remove all of them in a
> separate patch? 'len' in pgstat_recv_* functions has never been used
> since the stats collector code is introduced. It seems like that it
> was mistakenly introduced in the first commit and other pgstat_recv_*
> functions were added that followed it to define ‘len’ but didn’t also
> use it at all.
>
> >
> > 2) in 0003 patch
> >
> >   * Helper function for apply_handle_commit and apply_handle_stream_commit.
> >   */
> >  static void
> > -apply_handle_commit_internal(StringInfo s, LogicalRepCommitData *commit_data)
> > +apply_handle_commit_internal(LogicalRepCommitData *commit_data)
> >  {
> >
> > This looks like a separate change which remove unused paramater in existing
> > code, maybe we can get this committed first ?
>
> Yeah, it seems to be introduced by commit 0926e96c493. I've attached
> the patch for that.
>
> Also, I've attached the updated version patches. This version patch
> has pg_stat_reset_subscription_error() SQL function and sends a clear
> message after skipping the transaction. 0004 patch includes the
> skipping transaction feature and introducing RESET to ALTER
> SUBSCRIPTION. It would be better to separate them.
>

I've attached the new version patches that fix cfbot failure.


Regards,

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