Re: Skipping logical replication transactions on subscriber side

On Thu, Oct 21, 2021 at 10:30 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Oct 20, 2021 at 12:33 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
> >
> > On Mon, Oct 18, 2021 at 12:34 PM Masahiko Sawada <sawada.mshk@gmail.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:
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]

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();
+}

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

Regards,
Vignesh