On Tue, Feb 22, 2022 at 6:53 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Tue, Feb 22, 2022 at 11:15 AM Masahiko Sawada <sawada.m...@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/
