Re: pgstat include expansion

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