On Tue, Feb 17, 2026 at 4:44 PM Amit Kapila <[email protected]> wrote: > > On Tue, Feb 17, 2026 at 12:30 PM Nisha Moond <[email protected]> wrote: > > > > On Tue, Feb 17, 2026 at 10:15 AM Amit Kapila <[email protected]> > > wrote: > > > > > > On Mon, Feb 16, 2026 at 11:20 PM Andres Freund <[email protected]> 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
v2-0001-Add-get_logical_worker_type-to-retrieve-the-worke.patch
Description: Binary data
