On Mon, 16 Feb 2026 at 11:06, Amit Kapila <[email protected]> wrote: > > On Mon, Feb 16, 2026 at 4:49 AM Chao Li <[email protected]> wrote: > > > > > 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 > > > > > > > > > > ``` > > -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
