Subject: Re: pgstat include expansion

From: Alvaro Herrera <[email-masked]>
Date: Mon, 16 Feb 2026 17:32:27 +0100

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.

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 ?)

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"La espina, desde que nace, ya pincha" (Proverbio africano)

---

From: Andres Freund <[email-masked]>
Date: Mon, 16 Feb 2026 12:50:29 -0500

Hi,

> 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.

> 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

---

From: Amit Kapila <[email-masked]>
Date: Tue, 17 Feb 2026 10:15:07 +0530

>     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).

With Regards,
Amit Kapila.