Re: Skipping logical replication transactions on subscriber side - Mailing list pgsql-hackers

On Tue, Nov 9, 2021 at 3:07 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Sun, Nov 7, 2021 at 7:50 PM 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.
>
> While reviewing the v20, I have some initial comments,
>
> +     <row>
> +
<entry><structname>pg_stat_subscription_workers</structname><indexterm><primary>pg_stat_subscription_workers</primary></indexterm></entry>
> +      <entry>At least one row per subscription, showing about errors that
> +      occurred on subscription.
> +      See <link linkend="monitoring-pg-stat-subscription-workers">
> +      <structname>pg_stat_subscription_workers</structname></link> for details.
> +      </entry>
>
> 1.
> I don't like the fact that this view is very specific for showing the
> errors but the name of the view is very generic.  So are we keeping
> this name to expand the scope of the view in the future?  If this is
> meant only for showing the errors then the name should be more
> specific.

As Amit already mentioned, we're planning to add more xact statistics
to this view. I've mentioned that in the commit message.

>
> 2.
> Why comment says "At least one row per subscription"? this looks
> confusing, I mean if there is no error then there will not be even one
> row right?
>
>
> +  <para>
> +   The <structname>pg_stat_subscription_workers</structname> view will contain
> +   one row per subscription error reported by workers applying logical
> +   replication changes and workers handling the initial data copy of the
> +   subscribed tables.
> +  </para>

Right. Fixed.

>
> 3.
> So there will only be one row per subscription?  I did not read the
> code, but suppose there was an error due to some constraint now if
> that constraint is removed and there is a new error then the old error
> will be removed immediately or it will be removed by auto vacuum?  If
> it is not removed immediately then there could be multiple errors per
> subscription in the view so the comment is not correct.

There is one row per subscription worker (apply worker and tablesync
worker). If the same error consecutively occurred, error_count is
incremented and last_error_time is updated. Otherwise, i.g., if a
different error occurred on the apply worker, all statistics are
updated.

>
> 4.
> +     <row>
> +      <entry role="catalog_table_entry"><para role="column_definition">
> +       <structfield>last_error_time</structfield> <type>timestamp
> with time zone</type>
> +      </para>
> +      <para>
> +       Time at which the last error occurred
> +      </para></entry>
> +     </row>
>
> Will it be useful to know when the first time error occurred?

Good idea. Users can know when the subscription stopped due to this
error. Added.

>
> 5.
> +     <row>
> +      <entry role="catalog_table_entry"><para role="column_definition">
> +       <structfield>stats_reset</structfield> <type>timestamp with
> time zone</type>
> +      </para>
> +      <para>
>
> The actual view does not contain this column.

Removed.

>
> 6.
> +       <para>
> +        Resets statistics of a single subscription worker statistics.
>
> /Resets statistics of a single subscription worker statistics/Resets
> statistics of a single subscription worker

Fixed.

I'll update an updated patch soon.


Regards,

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