> On Feb 14, 2026, at 06:14, Andres Freund <[email protected]> wrote:
>
> Hi,
>
> A few recent-ish changes have substantially expanded the set of headers
> included in pgstat.h. As pgstat.h is pretty widely included, that strikes me
> as bad.
>
> commit f6a4c498dcf
> Author: Amit Kapila <[email protected]>
> Date: 2025-11-07 08:05:08 +0000
>
> Add seq_sync_error_count to subscription statistics.
>
> added an include of replication/worker_internal.h, which in turn includes a
> lot of other headers. It's also seems somewhat obvious that a header named
> _internal.h shouldn't be included in something as widely included as pgstat.h
>
>
> commit 7e638d7f509
> Author: Álvaro Herrera <[email protected]>
> Date: 2025-09-25 14:45:08 +0200
>
> Don't include execnodes.h in replication/conflict.h
>
> added an include of access/transam.h, which I don't love being included,
> although it's less bad than some of the other cases. It's not actually to
> blame for that though, as it shrank what was included noticeably.
>
>
> commit 6c2b5edecc0
> Author: Amit Kapila <[email protected]>
> Date: 2024-09-04 08:55:21 +0530
>
> Collect statistics about conflicts in logical replication.
>
> added an include of conflict.h, which in turn includes utils/timestamp.h,
> which includes fmgr.h (and a lot more, before 7e638d7f509).
>
>
> I think now that we rely on C11, we actually could also forward-declare enum
> typedefs. That would allow us to avoid including
> worker_internal.h. Unfortunately I think C++ might throw a wrench in the mix
> for that - IIUC it only allows forward declaring enums when using 'enum
> class', but I don't think we can rely on that. I think the best solution
> would be to move the typedef to a more suitable header, but baring that, I
> guess just passing an int would do.
>
>
> By forward declaring FullTransactionId, we can avoid including
> access/transam.h
>
>
> conflict.h includes utils/timestamp.h, even though it only needs the
> types. Using datatype/timestamp.h suffices (although it requires some fixups
> elsewhere).
>
>
> conflict.h includes nodes/pg_list.h, which does in turn include nodes/nodes.h,
> which, while not the worst, seems unnecessary. Another forward declaration
> solves that.
>
>
> The attached patch is just a prototype.
>
>
> I also don't like that pgstat.h includes backend_status.h, which includes
> things like pqcomm.h and miscadmin.h. But that's - leaving some moving around
> aside - of a lot longer standing. We probably should move BackendType out of
> miscadmin.h one of these days. Not sure what to do about the pqcomm.h
> include...
>
>
> Greetings,
>
> Andres Freund
> <v1-0001-WIP-Reduce-includes-in-pgstat.h.patch>
```
-pgstat_report_subscription_error(Oid subid, LogicalRepWorkerType wtype)
+pgstat_report_subscription_error(Oid subid, int wtype)
```
I am not a fan of changing LogicalRepWorkerType to int that feels like an
unnecessary trade-off. LogicalRepWorkerType is defined in worker_internal.h,
but as it’s used in pg_stat, it isn't “internal” anymore. So, maybe we should
move the enum to a new worker_types.h and include that instead?
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
