Re: Skipping logical replication transactions on subscriber side

On Mon, Nov 8, 2021 at 4:10 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Mon, Nov 8, 2021 at 1:20 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > I've attached an updated patch. In this version patch, subscription
> > worker statistics are collected per-database and handled in a similar
> > way to tables and functions. I think perhaps we still need to discuss
> > details of how the statistics should be handled but I'd like to share
> > the patch for discussion.
> >
>
> That's for the updated patch.
> Some initial comments on the v20 patch:

Thank you for the comments!

>
>
> doc/src/sgml/monitoring.sgml
>
> (1) wording
> The word "information" seems to be missing after "showing" (otherwise
> is reads "showing about errors", which isn't correct grammar).
> I suggest the following change:
>
> BEFORE:
> +      <entry>At least one row per subscription, showing about errors that
> +      occurred on subscription.
> AFTER:
> +      <entry>At least one row per subscription, showing information about
> +      errors that occurred on subscription.

Fixed.

>
> (2) pg_stat_reset_subscription_worker(subid Oid, relid Oid) function
> documentation
> The description doesn't read well. I'd suggest the following change:
>
> BEFORE:
> * Resets statistics of a single subscription worker statistics.
> AFTER:
> * Resets the statistics of a single subscription worker.
>
> I think that the documentation for this function should make it clear
> that a non-NULL "subid" parameter is required for both reset cases
> (tablesync and apply).
> Perhaps this could be done by simply changing the first sentence to say:
> "Resets the statistics of a single subscription worker, for a worker
> running on the subscription with <parameter>subid</parameter>."
> (and then can remove " running on the subscription with
> <parameter>subid</parameter>" from the last sentence)

Fixed.

>
> I think that the documentation for this function should say that it
> should be used in conjunction with the "pg_stat_subscription_workers"
> view in order to obtain the required subid/relid values for resetting.
> (and should provide a link to the documentation for that view)

I think it's not necessarily true that users should use
pg_stat_subscription_workers in order to obtain subid/relid since we
can obtain the same also from pg_subscription_rel. But I agree that it
should clarify that this function resets entries of
pg_stat_subscription view. Fixed.

>
> Also, I think that the function documentation should make it clear how
> to distinguish the tablesync vs apply worker statistics case.
> e.g. the tablesync error case is indicated by a null "command" in the
> information returned from the "pg_stat_subscription_workers" view
> (otherwise it seems a user could only know this by looking at the server log).

The documentation of pg_stat_subscription_workers explains that
subrelid is always NULL for apply workers. Is it not enough?

>
> Finally, there are currently no tests for this new function.

I've added some tests.

>
> (3) pg_stat_subscription_workers
> In the documentation for this, some users may not realise that "the
> initial data copy" refers to "tablesync", so maybe say "the initial
> data copy (tablesync)", or similar.
>

Perhaps it's better not to use the term "tablesync" since we don't use
the term anywhere now. Instead, we should say more clearly, say
"subscription worker handling initial data copy of the relation, as
the description pg_stat_subscription says.

> (4) stats_reset
> "stats_reset" is currently documented as the last column of the
> "pg_stat_subscription_workers" view - but it's actually no longer
> included in the view.

Removed.

>
> (5) src/tools/pgindent/typedefs.list
> The following current entries are bogus:
> PgStat_MsgSubWorkerErrorPurge
> PgStat_MsgSubWorkerPurge
>
> The following entry is missing:
> PgStat_MsgSubscriptionPurge

Fixed.

I'll submit an updated patch soon.

Regards,

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