On Sat, Feb 14, 2026 at 3:45 AM 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). >
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
v1-0001-Fix-header-inclusion-in-conflict.h-file.patch
Description: Binary data
