pgstat include expansion

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

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

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

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)
/messages/by-id/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]/messages/by-id/CAA4eK1+ZOXJotnk+d4mxHFP_i2SEWGTdyJgkQadOCvMgEcPGVg@mail.gmail.com -- Thanks, Nisha.

[1]: /messages/by-id/CAA4eK1+ZOXJotnk+d4mxHFP_i2SEWGTdyJgkQadOCvMgEcPGVg@mail.gmail.com -- Thanks, Nisha
--
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.