Re: Skipping logical replication transactions on subscriber side

On Thu, Oct 28, 2021 at 7:47 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Thu, Oct 21, 2021 at 10:30 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Wed, Oct 20, 2021 at 12:33 PM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
> > >
> > > On Mon, Oct 18, 2021 at 12:34 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > > >
> > > > I've attached updated patches that incorporate all comments I got so far.
> > > >
> > >
> > > Minor comment on patch 17-0003
> >
> > Thank you for the comment!
> >
> > >
> > > src/backend/replication/logical/worker.c
> > >
> > > (1) Typo in apply_handle_stream_abort() comment:
> > >
> > > /* Stop skipping transaction transaction, if enabled */
> > > should be:
> > > /* Stop skipping transaction changes, if enabled */
> >
> > Fixed.
> >
> > I've attached updated patches.
>
> I have started to have a look at the feature and review the patch, my
> initial comments:

Thank you for the comments!

> 1) I could specify invalid subscriber id to
> pg_stat_reset_subscription_worker which creates an assertion failure?
>
> +static void
> +pgstat_recv_resetsubworkercounter(PgStat_MsgResetsubworkercounter
> *msg, int len)
> +{
> + PgStat_StatSubWorkerEntry *wentry;
> +
> + Assert(OidIsValid(msg->m_subid));
> +
> + /* Get subscription worker stats */
> + wentry = pgstat_get_subworker_entry(msg->m_subid,
> msg->m_subrelid, false);
>
> postgres=# select pg_stat_reset_subscription_worker(NULL, NULL);
> pg_stat_reset_subscription_worker
> -----------------------------------
>
> (1 row)
>
> TRAP: FailedAssertion("OidIsValid(msg->m_subid)", File: "pgstat.c",
> Line: 5742, PID: 789588)
> postgres: stats collector (ExceptionalCondition+0xd0)[0x55d33bba4778]
> postgres: stats collector (+0x545a43)[0x55d33b90aa43]
> postgres: stats collector (+0x541fad)[0x55d33b906fad]
> postgres: stats collector (pgstat_start+0xdd)[0x55d33b9020e1]
> postgres: stats collector (+0x54ae0c)[0x55d33b90fe0c]
> /lib/x86_64-linux-gnu/libpthread.so.0(+0x141f0)[0x7f8509ccc1f0]
> /lib/x86_64-linux-gnu/libc.so.6(__select+0x57)[0x7f8509a78ac7]
> postgres: stats collector (+0x548cab)[0x55d33b90dcab]
> postgres: stats collector (PostmasterMain+0x134c)[0x55d33b90d5c6]
> postgres: stats collector (+0x43b8be)[0x55d33b8008be]
> /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xd5)[0x7f8509992565]
> postgres: stats collector (_start+0x2e)[0x55d33b48e4fe]

Good catch. Fixed.

>
> 2) I was able to provide invalid relation id for
> pg_stat_reset_subscription_worker? Should we add any validation for
> this?
> select pg_stat_reset_subscription_worker(16389, -1);
>
> +pg_stat_reset_subscription_worker(PG_FUNCTION_ARGS)
> +{
> + Oid subid = PG_GETARG_OID(0);
> + Oid relid;
> +
> + if (PG_ARGISNULL(1))
> + relid = InvalidOid; /* reset apply worker
> error stats */
> + else
> + relid = PG_GETARG_OID(1); /* reset table sync
> worker error stats */
> +
> + pgstat_reset_subworker_stats(subid, relid);
> +
> + PG_RETURN_VOID();
> +}

I think that validation is not necessarily necessary. OID '-1' is interpreted as
4294967295 and we don't reject it.

>
> 3) 025_error_report test is failing because of one of the recent
> commit that has made some changes in the way node is initialized in
> the tap tests, corresponding changes need to be done in
> 025_error_report:
> t/025_error_report.pl .............. Dubious, test returned 2 (wstat 512, 0x200)
> No subtests run
> t/100_bugs.pl ...................... ok

Fixed.

These comments are incorporated into the latest version patch I just
submitted[1].

Regards,

[1] https://www.postgresql.org/message-id/CAD21AoDY-9_x819F_m1_wfCVXXFJrGiSmR2MfC9Nw4nW8Om0qA%40mail.gmail.com

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