Re: pgstat include expansion

On Tue, Feb 17, 2026 at 4:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Feb 17, 2026 at 12:30 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> >
> > On Tue, Feb 17, 2026 at 10:15 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Mon, Feb 16, 2026 at 11:20 PM Andres Freund <andres@anarazel.de> wrote:
> > > >
> > > >
> > > > It doesn't seem like the right thing to have pgstat_subscription.c translate
> > > > the worker type to the concrete error this way. Afaict each of the callsites
> > > > of pgstat_report_subscription_error() actually knows what kind of error its
> > > > reporting, but just uses the worker type to do so.
> > > >
> > > > I'd either change the signature to have one argument for each of the error
> > > > types, i.e.
> > > >     pgstat_report_subscription_error(int subid,
> > > >                                      bool apply_error,
> > > >                                      bool sequencesync_error,
> > > >                                      bool_tablesync_error);
> > > >
> > > > or split the function into three, and have
> > > >     pgstat_report_subscription_{apply,sequence,tablesync}(int subid);
> > > >
> > >
> > > Good idea. +1 for the second approach to split the function. We can
> > > name them as pgstat_report_subscription_apply_error(int subid),
> > > pgstat_report_subscription_sequence_error(int subid),
> > > pgstat_report_subscription_tablesync_error(int subid).
> > >
> > Please find the attached patch implementing the idea. Introduced three
> > new worker specific functions as suggested and includes a few required
> > fixups after removing worker_internal.h from pgstat.h, as done earlier
> > in Amit’s patch [1].
> >
>
> *
> @@ -5606,8 +5606,10 @@ start_apply(XLogRecPtr origin_startpos)
>   * idle state.
>   */
>   AbortOutOfAnyTransaction();
> - pgstat_report_subscription_error(MySubscription->oid,
> - MyLogicalRepWorker->type);
> + if (am_tablesync_worker())
> + pgstat_report_subscription_tablesync_error(MySubscription->oid);
> + else
> + pgstat_report_subscription_apply_error(MySubscription->oid);
>
>   PG_RE_THROW();
>   }
> @@ -5960,8 +5962,12 @@ DisableSubscriptionAndExit(void)
>   * Report the worker failed during sequence synchronization, table
>   * synchronization, or apply.
>   */
> - pgstat_report_subscription_error(MyLogicalRepWorker->subid,
> - MyLogicalRepWorker->type);
> + if (am_tablesync_worker())
> + pgstat_report_subscription_tablesync_error(MySubscription->oid);
> + else if (am_sequencesync_worker())
> + pgstat_report_subscription_sequencesync_error(MySubscription->oid);
> + else
> + pgstat_report_subscription_apply_error(MySubscription->oid);
>
> As the worker type is not directly available in all places, the other
> possibility is to expose a function like GetLogicalWorkerType from
> worker_internal.h and use it directly in
> pgstat_report_subscription_error() instead of passing it via
> parameter. We can get the worker_type via MyLogicalRepWorker->type
> which should be accessible as it will be invoked by one of the logical
> replication workers.
>
+1
This seems like a cleaner approach than adding multiple worker
specific checks and functions.
Here is a patch that implements this idea.

--
Thanks,
Nisha