On Tue, Feb 22, 2022 at 6:53 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Feb 22, 2022 at 11:15 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > I've attached a patch that changes pg_stat_subscription_workers view.
> > It removes non-cumulative values such as error details such as
> > error-XID and the error message from the view, and consequently the
> > view now has only cumulative statistics counters: apply_error_count
> > and sync_error_count. Since the new view name is under discussion I
> > temporarily chose pg_stat_subscription_activity.
> >
>
> Few comments:
> =============
Thank you for the comments!
> 1.
> --- a/src/backend/catalog/system_functions.sql
> +++ b/src/backend/catalog/system_functions.sql
> @@ -637,11 +637,9 @@ REVOKE EXECUTE ON FUNCTION
> pg_stat_reset_single_table_counters(oid) FROM public;
>
> REVOKE EXECUTE ON FUNCTION
> pg_stat_reset_single_function_counters(oid) FROM public;
>
> -REVOKE EXECUTE ON FUNCTION pg_stat_reset_replication_slot(text) FROM public;
> -
> -REVOKE EXECUTE ON FUNCTION pg_stat_reset_subscription_worker(oid) FROM public;
> +REVOKE EXECUTE ON FUNCTION
> pg_stat_reset_single_subscription_counters(oid) FROM public;
>
> -REVOKE EXECUTE ON FUNCTION pg_stat_reset_subscription_worker(oid,
> oid) FROM public;
> +REVOKE EXECUTE ON FUNCTION pg_stat_reset_replication_slot(text) FROM public;
>
> Is there a need to change anything about
> pg_stat_reset_replication_slot() in this patch?
It doesn't change pg_stat_reset_replication_slot() but just changes
the order in order to put the modified function
pg_stat_reset_single_subscription_counters() closer to other similar
functions such as pg_stat_reset_single_function_counters().
>
> 2. Do we still need to use LATERAL in the view's query?
There are some functions that use LATERAL in a similar way but it
seems no need to put LATERAL before the function call. Will remove.
> 3. Can we send error stats pgstat_report_stat() as that will be called
> via proc_exit() path. We can set the phase (apply/sync) in
> apply_error_callback_arg and then use that to send the appropriate
> message. I think this will obviate the need for try..catch.
If we use pgstat_report_stat() to send subscription stats messages,
all processes end up going through that path. It might not bring
overhead in practice but I'd like to avoid it. And, since the apply
worker also calls pgstat_report_stat() at the end of the transaction,
we might need to change pgstat_report_stat() so that it doesn't send
the subscription messages when it gets called at the end of the
transaction. I think it's likely that PG_TRY() and PG_CATCH() wil be
added for example, when the disable_on_error feature or the storing
error details feature is introduced, so obviating the need for them at
this point would not benefit much.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
