Обсуждение: pgstat include expansion

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 <akapila@postgresql.org>
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 <alvherre@kurilemu.de>
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 <akapila@postgresql.org>
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

> On Feb 14, 2026, at 06:14, Andres Freund <andres@anarazel.de> 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 <akapila@postgresql.org>
> 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 <alvherre@kurilemu.de>
> 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 <akapila@postgresql.org>
> 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
isdefined in worker_internal.h, but as it’s used in pg_stat, it isn't “internal” anymore. So, maybe we should move the
enumto a new worker_types.h and include that instead? 

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Fri, Feb 13, 2026 at 05:14:01PM -0500, Andres Freund wrote:
> 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.


> -extern void pgstat_report_subscription_error(Oid subid,
> -                                             LogicalRepWorkerType wtype);
> +extern void pgstat_report_subscription_error(Oid subid, int wtype);

FWIW, I like some type enforcements when it comes to such report
routines.  That avoids some careless assignments.  This is usually a
sign of header refactoring to me, where the "light" declarations ought
to be moved into an independent header that can be fed back to other
places, like this one.  An enum declaration or a set of constants can
be usually worth a split if their knowledge gets a lot across the
tree.  That's just to say that while I agree about reducing the header
footprint, I don't find the result presented here to be the best thing
we can do.
--
Michael

On Mon, Feb 16, 2026 at 4:49 AM Chao Li <li.evan.chao@gmail.com> wrote:
>
> > On Feb 14, 2026, at 06:14, Andres Freund <andres@anarazel.de> 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 <akapila@postgresql.org>
> > 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
> >
> >
>
> ```
> -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
isdefined in worker_internal.h, but as it’s used in pg_stat, it isn't “internal” anymore. So, maybe we should move the
enumto a new worker_types.h and include that instead? 
>

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.

Fixed few warnings via different includes after the above change:
*
procsignal.c: In function ‘ProcessProcSignalBarrier’:
procsignal.c:580:61: warning: implicit declaration of function
‘ProcessBarrierUpdateXLogLogicalInfo’
[-Wimplicit-function-declaration]
  580 |                                                 processed =
ProcessBarrierUpdateXLogLogicalInfo();
      |
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This warning appears because ProcessBarrierUpdateXLogLogicalInfo
declaration present in logicalctl.h was included in procsignal.c
through pgstat.h-
>worker_internal->walreceiver.h->xlog.h->logicalctl.h. Since the proposed patch removes worker_internal.h from pgstat.h
thiswarning appears. For this, we need to include logicalctl.h in procsignal.c. 

*
functioncmds.c: In function ‘compute_return_type’:
functioncmds.c:157:17: warning: implicit declaration of function
‘CommandCounterIncrement’ [-Wimplicit-function-declaration]
  157 |                 CommandCounterIncrement();


This warning appears because CommandCounterIncrement declaration
present in xact.h was getting included in functioncmds.c through
pgstat.h->worker_internal.h->logicalrelation.h->logicalproto.h->xact.h.
to fix this, we need to include "access/xact.h" in functioncmds.c.

*
On Windows:
[68/196] Compiling C object
src/test/modules/test_custom_stats/test_custom_var_stats.dll.p/test_custom_var_stats.c.obj
../src/test/modules/test_custom_stats/test_custom_var_stats.c(685):
warning C4047: '=': 'HeapTuple' differs in levels of indirection from
'int'

Previously HeapTuple seems to be detected via
pgstat_internal.h->pgstat.h->worker_internal.h->logicalrelation.h->index.h->execnodes.h->tupconvert.h->htup.h.
To fix, we need to include htup_details.h in test_custom_var_stats.c
similar to test_custom_fixed_stats.c.

--
With Regards,
Amit Kapila.

On Mon, 16 Feb 2026 at 11:06, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Feb 16, 2026 at 4:49 AM Chao Li <li.evan.chao@gmail.com> wrote:
> >
> > > On Feb 14, 2026, at 06:14, Andres Freund <andres@anarazel.de> 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 <akapila@postgresql.org>
> > > 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
> > >
> > >
> >
> > ```
> > -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.
LogicalRepWorkerTypeis defined in worker_internal.h, but as it’s used in pg_stat, it isn't “internal” anymore. So,
maybewe should move the enum to a new worker_types.h and include that instead? 
> >
>
> 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.
>
> Fixed few warnings via different includes after the above change:
> *
> procsignal.c: In function ‘ProcessProcSignalBarrier’:
> procsignal.c:580:61: warning: implicit declaration of function
> ‘ProcessBarrierUpdateXLogLogicalInfo’
> [-Wimplicit-function-declaration]
>   580 |                                                 processed =
> ProcessBarrierUpdateXLogLogicalInfo();
>       |
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> This warning appears because ProcessBarrierUpdateXLogLogicalInfo
> declaration present in logicalctl.h was included in procsignal.c
> through pgstat.h-
> >worker_internal->walreceiver.h->xlog.h->logicalctl.h. Since the proposed patch removes worker_internal.h from
pgstat.hthis warning appears. For this, we need to include logicalctl.h in procsignal.c. 
>
> *
> functioncmds.c: In function ‘compute_return_type’:
> functioncmds.c:157:17: warning: implicit declaration of function
> ‘CommandCounterIncrement’ [-Wimplicit-function-declaration]
>   157 |                 CommandCounterIncrement();
>
>
> This warning appears because CommandCounterIncrement declaration
> present in xact.h was getting included in functioncmds.c through
> pgstat.h->worker_internal.h->logicalrelation.h->logicalproto.h->xact.h.
> to fix this, we need to include "access/xact.h" in functioncmds.c.
>
> *
> On Windows:
> [68/196] Compiling C object
> src/test/modules/test_custom_stats/test_custom_var_stats.dll.p/test_custom_var_stats.c.obj
> ../src/test/modules/test_custom_stats/test_custom_var_stats.c(685):
> warning C4047: '=': 'HeapTuple' differs in levels of indirection from
> 'int'
>
> Previously HeapTuple seems to be detected via
> pgstat_internal.h->pgstat.h->worker_internal.h->logicalrelation.h->index.h->execnodes.h->tupconvert.h->htup.h.
> To fix, we need to include htup_details.h in test_custom_var_stats.c
> similar to test_custom_fixed_stats.c.

One small comment, we should include access/xact.h after access/table.h:
diff --git a/src/backend/commands/functioncmds.c
b/src/backend/commands/functioncmds.c
index a516b037dea..bcac267b78c 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -32,6 +32,7 @@
  */
 #include "postgres.h"

+#include "access/xact.h"
 #include "access/htup_details.h"
 #include "access/table.h"
 #include "catalog/catalog.h"

Regards,
Vignesh

On Sat, Feb 14, 2026 at 3:45 AM Andres Freund <andres@anarazel.de> 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 <akapila@postgresql.org>
> 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 <alvherre@kurilemu.de>
> 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 <akapila@postgresql.org>
> 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).
>

It works even without including "utils/timestamp.h" in conflict.h, as
removing this header still allows the build and full tests to pass.
This is likely because the required definitions are included
indirectly, as all files using conflict.h already include either
"access/commit_ts.h" (which includes "datatype/timestamp.h") or
"datatype/timestamp.h" itself.

To avoid possible issues in the future, if conflict.h is used in a
file that does not include timestamp.h, it would be safer to include
"datatype/timestamp.h" directly, which is sufficient and requires no
fixups elsewhere.
The attached patch includes the required change, and make check-world
passes cleanly for it.

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

I am fine either way here.

--
Thanks,
Nisha

On Mon, Feb 16, 2026 at 11:05:41AM +0530, 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.

Yep, this feels much cleaner.  I am not sure we really need to limit
LogicalRepWorkerType to be in an internal header knowing that it is
exposed in the pgstat layer.
--
Michael

On 2026-Feb-13, Andres Freund wrote:

> commit 7e638d7f509
> Author: Álvaro Herrera <alvherre@kurilemu.de>
> 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.


> diff --git a/src/include/pgstat.h b/src/include/pgstat.h
> index fff7ecc2533..a2021a0e66f 100644
> --- a/src/include/pgstat.h
> +++ b/src/include/pgstat.h
> @@ -11,18 +11,20 @@
>  #ifndef PGSTAT_H
>  #define PGSTAT_H
>  
> -#include "access/transam.h"        /* for FullTransactionId */
>  #include "datatype/timestamp.h"
>  #include "portability/instr_time.h"
>  #include "postmaster/pgarch.h"    /* for MAX_XFN_CHARS */
> -#include "replication/conflict.h"
> -#include "replication/worker_internal.h"
> +#include "replication/conflict.h"    /* for CONFLICT_NUM_TYPES */
>  #include "utils/backend_progress.h" /* for backward compatibility */    /* IWYU pragma: export */
>  #include "utils/backend_status.h"    /* for backward compatibility */    /* IWYU pragma: export */
>  #include "utils/pgstat_kind.h"
> -#include "utils/relcache.h"
>  #include "utils/wait_event.h"    /* for backward compatibility */    /* IWYU pragma: export */
>  
> +/* to avoid including access/transam.h, for FullTransactionId */
> +typedef struct FullTransactionId FullTransactionId;
> +
> +/* to avoid including utils/relcache.h */
> +typedef struct RelationData *Relation;
>  
>  /* ----------
>   * Paths for the statistics files (relative to installation's $PGDATA).

Yeah, I intended not to add access/transam.h here and instead
forward-declare FullTransactionId, but forgot to actually remove it
before pushing.  So +1 for what you're doing here with that.  Same with
relcache.h and most of the other changes you're doing here.

I don't think the removal of pg_list.h works terribly well though, as
that is something that I would expect to be pretty much everywhere, so
it seems somewhat pointless to try to avoid it in this file.

I'm not very sure what to do about LogicalRepWorkerType ... will
follow-up to Amit's reply.

I think we should in addition remove some includes that were kept during
recent IWYU hacking "for backwards compatibility", and instead include
them explicitly wherever they are needed.  The attached does that.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"You're _really_ hosed if the person doing the hiring doesn't understand
relational systems: you end up with a whole raft of programmers, none of
whom has had a Date with the clue stick."              (Andrew Sullivan)
https://postgr.es/m/20050809113420.GD2768@phlogiston.dyndns.org

On 2026-Feb-16, Alvaro Herrera wrote:

> I think we should in addition remove some includes that were kept during
> recent IWYU hacking "for backwards compatibility", and instead include
> them explicitly wherever they are needed.  The attached does that.

Sigh, I forgot the attachments.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

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)

Hi,

On 2026-02-16 13:36:02 +0900, Michael Paquier wrote:
> On Fri, Feb 13, 2026 at 05:14:01PM -0500, Andres Freund wrote:
> > 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.
> 
> 
> > -extern void pgstat_report_subscription_error(Oid subid,
> > -                                             LogicalRepWorkerType wtype);
> > +extern void pgstat_report_subscription_error(Oid subid, int wtype);
> 
> FWIW, I like some type enforcements when it comes to such report
> routines.  That avoids some careless assignments.

In theory I agree (I after all did suggest moving the typedef to a better
header).  However, the type enforcement argument IMO is somewhat bogus, as C
doesn't really have strong enum types, therefore you can just pass pretty
random stuff.  I do wish C enums could be stronger...


> This is usually a sign of header refactoring to me, where the "light"
> declarations ought to be moved into an independent header that can be fed
> back to other places, like this one.  An enum declaration or a set of
> constants can be usually worth a split if their knowledge gets a lot across
> the tree.  That's just to say that while I agree about reducing the header
> footprint, I don't find the result presented here to be the best thing we
> can do.

I literally wrote that it isn't the best solution in the quoted portion above
and suggested moving the typedef to a different header?  I was hoping the
author of the code would do that...

Greetings,

Andres Freund

Hi,

On 2026-02-16 17:18:36 +0100, Alvaro Herrera wrote:
> Yeah, I intended not to add access/transam.h here and instead
> forward-declare FullTransactionId, but forgot to actually remove it
> before pushing.  So +1 for what you're doing here with that.  Same with
> relcache.h and most of the other changes you're doing here.

Ah, great.


> I don't think the removal of pg_list.h works terribly well though, as
> that is something that I would expect to be pretty much everywhere, so
> it seems somewhat pointless to try to avoid it in this file.

Yea, I was on the fence about that one.

It does seem pretty crappy that pg_list.h is so widely included and in turn
includes nodes/nodes.h, which then depends on the generated nodetags.h.  For
one it forces an unnecessary rebuild of too much if you add a new node type,
but it also just aesthetically feels wrong.

But avoiding it just here does, as you say, not save very much.



> I think we should in addition remove some includes that were kept during
> recent IWYU hacking "for backwards compatibility", and instead include
> them explicitly wherever they are needed.  The attached does that.

FWIW, the "for backwards compatibility" includes are from prep work towards
shared memory stats. Before that all the stuff in backend_status.h,
backend_progress.h and wait_event.h was in pgstat.h, which was quite
unwieldy. The creation of those headers would have broken a lot of extensions
if we had not provided those backward compat includes from pgstat.h.

I suspect it would still break a fair number of extensions if removed those
includes today. Not sure if the improvement is worth it?  But even if we
decide not to remove the backward compat includes, I think we should fix our
own code to include the right headers directly, rather than rely on doing so
via pgstat.h.


FWIW, your patch that you sent subsequently doesn't seem to apply cleanly for
me? I think it's perhaps based on an older tree, it doesn't know about the
conflict.h include yet...  And it fails to build e.g. due to miscadmin.h
defines around BackendType not being indirectly included in pgstat.h anymore,
but there are many other failures...

Greetings,

Andres Freund

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

Hello,

On 2026-Feb-16, Andres Freund wrote:

> On 2026-02-16 17:18:36 +0100, Alvaro Herrera wrote:

> > I don't think the removal of pg_list.h works terribly well though, as
> > that is something that I would expect to be pretty much everywhere, so
> > it seems somewhat pointless to try to avoid it in this file.
> 
> Yea, I was on the fence about that one.
> 
> It does seem pretty crappy that pg_list.h is so widely included and in turn
> includes nodes/nodes.h, which then depends on the generated nodetags.h.  For
> one it forces an unnecessary rebuild of too much if you add a new node type,
> but it also just aesthetically feels wrong.

Yeah, I was unhappy about pg_list.h too on my recent header hacking, and
perhaps we can improve this somehow, but I think it's a bigger effort.

> > I think we should in addition remove some includes that were kept during
> > recent IWYU hacking "for backwards compatibility", and instead include
> > them explicitly wherever they are needed.  The attached does that.
> 
> FWIW, the "for backwards compatibility" includes are from prep work towards
> shared memory stats. Before that all the stuff in backend_status.h,
> backend_progress.h and wait_event.h was in pgstat.h, which was quite
> unwieldy.

Ah, okay.

> The creation of those headers would have broken a lot of extensions if
> we had not provided those backward compat includes from pgstat.h.

Right ... TBH the real issue for me is that wait_event.h includes the
generated file wait_event_types.h (same complaint you had above), but I
decided that if I was wielding a weapon against pgstat.h, it could as
well be a shotgun.  I guess I would be satisfied if we remove
wait_events.h from pgstat.h, and that probably won't break as many
extensions and won't have as much of a fallout.  Although TB further H,
I don't really see it as terribly bad if we break extensions with this
kind of work, since the fix on their side is fairly easy, noninvasive,
and doesn't have to be done with any urgency.

> FWIW, your patch that you sent subsequently doesn't seem to apply cleanly for
> me? I think it's perhaps based on an older tree, it doesn't know about the
> conflict.h include yet...  And it fails to build e.g. due to miscadmin.h
> defines around BackendType not being indirectly included in pgstat.h anymore,
> but there are many other failures...

Yeah, I just wrote it on (almost) clean master, but feel free to push
your changes whenever, and I'll rebase on top of that.  I suspect the
failures just mean that the implicit header inclusions are worse than
they appear at first.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On Mon, Feb 16, 2026 at 11:20 PM Andres Freund <andres@anarazel.de> wrote:
>
> 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);
>

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.

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:
> >
> > 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);
> >
>
> 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].

[1] https://www.postgresql.org/message-id/CAA4eK1%2BZOXJotnk%2Bd4mxHFP_i2SEWGTdyJgkQadOCvMgEcPGVg%40mail.gmail.com
--
Thanks,
Nisha

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.

--
With Regards,
Amit Kapila.

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

On 2026-Feb-16, Alvaro Herrera wrote:

> > FWIW, your patch that you sent subsequently doesn't seem to apply
> > cleanly for me? I think it's perhaps based on an older tree, it
> > doesn't know about the conflict.h include yet...  And it fails to
> > build e.g. due to miscadmin.h defines around BackendType not being
> > indirectly included in pgstat.h anymore, but there are many other
> > failures...
> 
> Yeah, I just wrote it on (almost) clean master, but feel free to push
> your changes whenever, and I'll rebase on top of that.

Actually, being more surgical and removing only wait_event.h, the
attached patch applies and compiles cleanly -- after yours this time,
rather than on master.  I think this is the most significant change
because of it including the generated file.  Given that pgstat.h is not
_so_ widely used, I think leaving the other includes there doesn't hurt
all that much anyway.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
<Schwern> It does it in a really, really complicated way
<crab> why does it need to be complicated?
<Schwern> Because it's MakeMaker.

On Wed, Feb 18, 2026 at 10:09 AM Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> On Tue, Feb 17, 2026 at 4:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > 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.
>

LGTM. I'll push this tomorrow unless there are comments/objections on
this patch.

--
With Regards,
Amit Kapila.

On Mon, Feb 16, 2026 at 1:18 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> On Sat, Feb 14, 2026 at 3:45 AM Andres Freund <andres@anarazel.de> wrote:
> >
> >
> > conflict.h includes utils/timestamp.h, even though it only needs the
> > types. Using datatype/timestamp.h suffices (although it requires some fixups
> > elsewhere).
> >
>
> It works even without including "utils/timestamp.h" in conflict.h, as
> removing this header still allows the build and full tests to pass.
> This is likely because the required definitions are included
> indirectly, as all files using conflict.h already include either
> "access/commit_ts.h" (which includes "datatype/timestamp.h") or
> "datatype/timestamp.h" itself.
>
> To avoid possible issues in the future, if conflict.h is used in a
> file that does not include timestamp.h, it would be safer to include
> "datatype/timestamp.h" directly, which is sufficient and requires no
> fixups elsewhere.
>

Yeah, that sounds better.

> The attached patch includes the required change, and make check-world
> passes cleanly for it.
>

Good. I have not tested it myself but if others don't see a problem
with this, I can do the tests and push it.

--
With Regards,
Amit Kapila.

On Wed, Feb 18, 2026 at 8:40 PM Alvaro Herrera <alvherre@kurilemu.de> wrote:
>
> On 2026-Feb-16, Alvaro Herrera wrote:
>
> > > FWIW, your patch that you sent subsequently doesn't seem to apply
> > > cleanly for me? I think it's perhaps based on an older tree, it
> > > doesn't know about the conflict.h include yet...  And it fails to
> > > build e.g. due to miscadmin.h defines around BackendType not being
> > > indirectly included in pgstat.h anymore, but there are many other
> > > failures...
> >
> > Yeah, I just wrote it on (almost) clean master, but feel free to push
> > your changes whenever, and I'll rebase on top of that.
>
> Actually, being more surgical and removing only wait_event.h, the
> attached patch applies and compiles cleanly -- after yours this time,
> rather than on master.  I think this is the most significant change
> because of it including the generated file.  Given that pgstat.h is not
> _so_ widely used, I think leaving the other includes there doesn't hurt
> all that much anyway.
>

After pushing the patch for removing worker_internal.h from pgstat.h,
I tried this patch, but it failed to apply for pgstat.h. I fixed that
manually and tried to build on mac and windows. I see the following
problems.

On Windows (using meson build):
==========================
[951/2089] Compiling C object
src/backend/postgres_lib.a.p/utils_adt_pgstatfuncs.c.obj
../src/backend/utils/adt/pgstatfuncs.c(473): warning C4047: '=':
'const char *' differs in levels of indirection from 'int'
../src/backend/utils/adt/pgstatfuncs.c(474): warning C4047: '=':
'const char *' differs in levels of indirection from 'int'
../src/backend/utils/adt/pgstatfuncs.c(833): warning C4047: '=':
'const char *' differs in levels of indirection from 'int'
../src/backend/utils/adt/pgstatfuncs.c(860): warning C4047: '=':
'const char *' differs in levels of indirection from 'int'
...
...
[1472/2089] Linking target src/backend/postgres.exe
FAILED: [code=1120] src/backend/postgres.exe src/backend/postgres.pdb
"link" @src/backend/postgres.exe.rsp
   Creating library src\backend\postgres.lib
access_transam_xact.c.obj : error LNK2019: unresolved external symbol
pgstat_report_wait_end referenced in function AbortTransaction
postmaster_auxprocess.c.obj : error LNK2001: unresolved external
symbol pgstat_report_wait_end
storage_file_fd.c.obj : error LNK2001: unresolved external symbol
pgstat_report_wait_end
storage_ipc_waiteventset.c.obj : error LNK2001: unresolved external
symbol pgstat_report_wait_end
storage_file_fd.c.obj : error LNK2019: unresolved external symbol
pgstat_report_wait_start referenced in function FileReadV
storage_ipc_waiteventset.c.obj : error LNK2001: unresolved external
symbol pgstat_report_wait_start
src\backend\postgres.exe : fatal error LNK1120: 2 unresolved externals

On Mac:
=======
xact.c:2855:2: error: call to undeclared function
'pgstat_report_wait_end'; ISO C99 and later do not support implicit
function declarations [-Wimplicit-function-declaration]
 2855 |         pgstat_report_wait_end();
      |         ^
xact.c:5263:2: error: call to undeclared function
'pgstat_report_wait_end'; ISO C99 and later do not support implicit
function declarations [-Wimplicit-function-declaration]
 5263 |         pgstat_report_wait_end();
      |         ^
2 errors generated.

Do let me know if everything is good at your end, I can retry to see
if there are any dev-environment issues at my end.

--
With Regards,
Amit Kapila.

On Thu, Feb 19, 2026 at 11:46 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Feb 16, 2026 at 1:18 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> >
>
> > The attached patch includes the required change, and make check-world
> > passes cleanly for it.
> >
>
> Good. I have not tested it myself but if others don't see a problem
> with this, I can do the tests and push it.
>

This change requires explicitly including utils/timestamp.h in
test_custom_fixed_stats.c, which previously relied on the indirect
inclusion. I made that change and pushed the patch.

--
With Regards,
Amit Kapila.

Hello,

I committed the removal of transam.h and relcache.h from pgstat.h now.
No other files needed adjustment for these changes.

Thanks

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"La fuerza no está en los medios físicos
sino que reside en una voluntad indomable" (Gandhi)

Hello

This one removes wait_event.h from pgstat.h.  The only difference from
the previous one, aside from the fact that I had to add pgstat.h to a
few more .c files, is that xlogreader.c needs to include it inside the
#ifndef FRONTEND; otherwise, "ninja -t missingdeps" complain about the
file being used in pg_rewind and pg_waldump (seen in CI).

I think this would conclude the current exercise.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

Hi,

Thanks for pushing the pgstat.h slimdown patch - I should have gotten to that
... but didn't.

On 2026-02-26 14:52:10 +0100, Alvaro Herrera wrote:
> This one removes wait_event.h from pgstat.h.  The only difference from
> the previous one, aside from the fact that I had to add pgstat.h to a
> few more .c files, is that xlogreader.c needs to include it inside the
> #ifndef FRONTEND; otherwise, "ninja -t missingdeps" complain about the
> file being used in pg_rewind and pg_waldump (seen in CI).

I'm somewhat worried about the amount of breakage this will cause in
extensions.  Many extensions use something like
   WaitLatch(wait_event_info = WAIT_EVENT_EXTENSION)
and many of them won't have included wait_event.h.

OTOH, Heikki's interrupt patch will already cause widespread breakage in just
such code, so maybe this is the time to do it.

Greetings,

Andres Freund

On 2026-Feb-26, Andres Freund wrote:

> On 2026-02-26 14:52:10 +0100, Alvaro Herrera wrote:
> > This one removes wait_event.h from pgstat.h.  The only difference from
> > the previous one, aside from the fact that I had to add pgstat.h to a
> > few more .c files, is that xlogreader.c needs to include it inside the
> > #ifndef FRONTEND; otherwise, "ninja -t missingdeps" complain about the
> > file being used in pg_rewind and pg_waldump (seen in CI).
> 
> I'm somewhat worried about the amount of breakage this will cause in
> extensions.  Many extensions use something like
>    WaitLatch(wait_event_info = WAIT_EVENT_EXTENSION)
> and many of them won't have included wait_event.h.

Yeah, I considered that, and instead of breaking these, to include
wait_event.h in latch.h.  This sounds a very reasonable thing to do.
But then I searched for other headers that have a wait_event_info
parameter in one of their functions, and if we force those to also
acquire wait_event.h, then the pollution is much worse than if we just
leave it in pgstat.h.  (condition_variable.h, fd.h, proc.h, barrier.h). 

The problem is that some of those headers are in turn being included by
others; most notably condition_variable.h is used in execnodes.h and
others, and that makes a huge mess.  (And it can't be removed.)

However, maybe we can just not be so principled about it, add it to
latch.h, and then we don't break the whole world.  I'll try that.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Finally, the phrase, 'No one was ever fired for buying an IBM' I don't believe
has ever been translated into German."                       (Leonard Tramiel)

On 2026-Feb-26, Alvaro Herrera wrote:

> However, maybe we can just not be so principled about it, add it to
> latch.h, and then we don't break the whole world.  I'll try that.

This one does it that way.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Cuando mañana llegue pelearemos segun lo que mañana exija" (Mowgli)

Hi,

On 2026-02-26 16:59:12 +0100, Alvaro Herrera wrote:
> On 2026-Feb-26, Andres Freund wrote:
> > On 2026-02-26 14:52:10 +0100, Alvaro Herrera wrote:
> > > This one removes wait_event.h from pgstat.h.  The only difference from
> > > the previous one, aside from the fact that I had to add pgstat.h to a
> > > few more .c files, is that xlogreader.c needs to include it inside the
> > > #ifndef FRONTEND; otherwise, "ninja -t missingdeps" complain about the
> > > file being used in pg_rewind and pg_waldump (seen in CI).
> >
> > I'm somewhat worried about the amount of breakage this will cause in
> > extensions.  Many extensions use something like
> >    WaitLatch(wait_event_info = WAIT_EVENT_EXTENSION)
> > and many of them won't have included wait_event.h.
>
> Yeah, I considered that, and instead of breaking these, to include
> wait_event.h in latch.h.  This sounds a very reasonable thing to do.
> But then I searched for other headers that have a wait_event_info
> parameter in one of their functions, and if we force those to also
> acquire wait_event.h, then the pollution is much worse than if we just
> leave it in pgstat.h.  (condition_variable.h, fd.h, proc.h, barrier.h).
>
> The problem is that some of those headers are in turn being included by
> others; most notably condition_variable.h is used in execnodes.h and
> others, and that makes a huge mess.  (And it can't be removed.)
>
> However, maybe we can just not be so principled about it, add it to
> latch.h, and then we don't break the whole world.  I'll try that.

FWIW, regardless of what we choose here (i.e. whether and where to
"unnecessarily" include wait_event.h), I'd be in favor of fixing all the
fallout in our source tree, just for cleanliness' sake. It's also presumably
the part of the patch that would be the most pain to keep updated.


I'm not sure that including in latch.h really would an improvement though -
that's included in proc.h and libpq.h, which both are fairly widely included
[1].


FWIW, I'm a lot less concerned with extension users of condition_variable.h,
fd.h, proc.h, barrier.h breaking due to removal of an implicit include, I
don't think they are remotely as widely used.


Greetings,

Andres Freund

[1] Although at least the include in libpq.h seems like it could trivially be
replaced by a forward declaration. There's only three .c files that need to be
fixed for that...

Hi,

On 2026-Feb-26, Andres Freund wrote:

> I'm not sure that including in latch.h really would an improvement though -
> that's included in proc.h and libpq.h, which both are fairly widely included
> [1].

> [1] Although at least the include in libpq.h seems like it could trivially be
> replaced by a forward declaration. There's only three .c files that need to be
> fixed for that...

Okay, let's start there -- pushed that for now.

Pity, that we can't remove latch.h from proc.h.  That's really where
most of the mess is, here ...

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
Essentially, you're proposing Kevlar shoes as a solution for the problem
that you want to walk around carrying a loaded gun aimed at your foot.
(Tom Lane)

On 2026-Feb-26, Andres Freund wrote:

> I'm not sure that including in latch.h really would an improvement though -
> that's included in proc.h and libpq.h, which both are fairly widely included
> [1].

Although we cannot remove latch.h, what we can do is remove proc.h from
shm_mq.h, which is actually very good because the proliferation of stuff
from there is enormous.  See the attached.  Practically all the network
of dependents of proc.h comes from there.  The fallout is even somewhat
insane.  I think this is a good cleanup.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Industry suffers from the managerial dogma that for the sake of stability
and continuity, the company should be independent of the competence of
individual employees."                                      (E. Dijkstra)

Hi,

On 2026-Feb-26, Andres Freund wrote:

> FWIW, regardless of what we choose here (i.e. whether and where to
> "unnecessarily" include wait_event.h), I'd be in favor of fixing all the
> fallout in our source tree, just for cleanliness' sake. It's also presumably
> the part of the patch that would be the most pain to keep updated.

Oh, that makes sense.

> I'm not sure that including in latch.h really would an improvement though -
> that's included in proc.h and libpq.h, which both are fairly widely included
> [1].

I have committed fixes for both of those, so that including an extra
file in latch.h is not as bad.

Here's one more patch.  I noticed that the call actually uses
PG_WAIT_EXTENSION, which is in storage/wait_classes.h.  Including that
seems a bit narrow-minded even if it's just for backwards compatibility,
so I decided to do wait_event_types.h instead.  This is still a bit
better than the whole wait_event.h IMO.  And also now it's IWYU: export
rather than IWYU: keep, which seems sensible.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"El miedo atento y previsor es la madre de la seguridad" (E. Burke)

On 2026-Feb-27, Alvaro Herrera wrote:

> Here's one more patch.  I noticed that the call actually uses
> PG_WAIT_EXTENSION, which is in storage/wait_classes.h.  Including that
> seems a bit narrow-minded even if it's just for backwards compatibility,
> so I decided to do wait_event_types.h instead.  This is still a bit
> better than the whole wait_event.h IMO.  And also now it's IWYU: export
> rather than IWYU: keep, which seems sensible.

Actually, I think this is not a great idea, because if we do that, then
wait_event_types.h (the generated header) is, overall, included in more
files than before, so it'd be a net loss.  But if we include
wait_classes.h in latch.h instead, then that aspect is definitely much
better, and I can't find anything that gets worse.  (Any code that wants
to use other wait events can include wait_event_types.h or wait_event.h
themselves).  So I propose to close this with the attached patch.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"The important things in the world are problems with society that we don't
understand at all. The machines will become more complicated but they won't
be more complicated than the societies that run them."    (Freeman Dyson)