Re: Skipping logical replication transactions on subscriber side

On Thu, Jul 29, 2021 at 2:04 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> 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.

Sorry I've attached wrong ones. Reattached the correct version patches.

Regards,

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