Re: Skipping logical replication transactions on subscriber side

On Thu, Jul 22, 2021 at 8:53 PM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On July 20, 2021 9:26 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > On Mon, Jul 19, 2021 at 8:38 PM houzj(dot)fnst(at)fujitsu(dot)com
> > <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > >
> > > On July 19, 2021 2:40 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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.

Regards,

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