Hi,
On 2026-02-16 17:32:27 +0100, Alvaro Herrera wrote:
> On 2026-Feb-16, Amit Kapila wrote:
>
> > How about moving LogicalRepWorkerType to logicalworker.h as in the
> > attached and then include that in pgstat.h? This requires few other
> > adjustments as previously some of the includes were working indirectly
> > via worker_internal.h.
>
> Yeah, I think the inclusion of worker_internal.h in pgstat.h is
> catastrophic, and the additions of .h files that you propose to fix them
> after the removal look OK to me, though I didn't try to compile or run
> headerscheck, though you should before pushing.
> I'm not sure about including logicalworker.h in pgstat.h though, given
> the prototypes you have there ... the logical worker type enum does not
> fit together with those IMO.
It sometimes is unavoidable, because of needing to influence the size of an
array or such, but that's not the case here.
> Maybe it'd be better to add a new file for pgstat_subscription.c and
> pgstat.h to use, where this enum lives. (Maybe something like
> src/include/replication/pgstat_worker.h or
> src/include/replication/worker_stat.h ?)
I think we should simply not pass the worker type. The only reason the worker
type passed is that that is used as a proxy for what kind of error occurred:
/*
* Report a subscription error.
*/
void
pgstat_report_subscription_error(Oid subid, int wtype)
{
PgStat_EntryRef *entry_ref;
PgStat_BackendSubEntry *pending;
entry_ref = pgstat_prep_pending_entry(PGSTAT_KIND_SUBSCRIPTION,
InvalidOid, subid, NULL);
pending = entry_ref->pending;
switch ((LogicalRepWorkerType) wtype)
{
case WORKERTYPE_APPLY:
pending->apply_error_count++;
break;
case WORKERTYPE_SEQUENCESYNC:
pending->sync_seq_error_count++;
break;
case WORKERTYPE_TABLESYNC:
pending->sync_table_error_count++;
break;
default:
/* Should never happen. */
Assert(0);
break;
}
}
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);
Greetings,
Andres Freund
