Horton

Thread

Commits

  1. Fix use-after-free in pgstat_fetch_stat_backend_by_pid()

  2. Remove initialization from PendingBackendStats

  3. Add WAL data to backend statistics

  4. Improve check for detection of pending data in backend statistics

  5. Fix some gaps in pg_stat_io with WAL receiver and WAL summarizer

  6. Handle auxiliary processes in SQL functions of backend statistics

  7. Invent pgstat_fetch_stat_backend_by_pid()

  8. Refactor code of pg_stat_get_wal() building result tuple

  9. Adding new PgStat_WalCounters structure in pgstat.h

  10. Remove pgstat_flush_wal()

  11. Refactor some code related to backend statistics

  12. Add backend-level statistics to pgstats

  1. per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-01-07T08:48:51Z

    Hi hackers,

Now that commit 9aea73fc61 added backend-level statistics to pgstats (and
per backend IO statistics), we can more easily add per backend statistics.

Please find attached a patch to implement $SUBJECT.

It's using the same layer as pg_stat_wal, except that it is now possible to know
how much WAL activity is happening in each backend rather than an overall
aggregate of all the activity.

A function called pg_stat_get_backend_wal() is added to access this data
depending on the PID of a backend.

=== Outcome ===

With this in place, one could for example:

1. Get the WAL statistics for a backend pid:

postgres=# select * from pg_stat_get_backend_wal(473278);
-[ RECORD 1 ]----+---------
wal_records      | 300008
wal_fpi          | 7
wal_bytes        | 17753097
wal_buffers_full | 933
wal_write        | 937
wal_sync         | 2
wal_write_time   | 0
wal_sync_time    | 0
stats_reset      |

2. Get the wal_bytes generated by application:

postgres=# SELECT application_name, wal_bytes, round(100 * wal_bytes/sum(wal_bytes) over(),2) AS "%"
FROM  (SELECT application_name, sum(wal_bytes) AS wal_bytes
       FROM pg_stat_activity, pg_stat_get_backend_wal(pid)
       WHERE wal_bytes != 0 GROUP BY application_name);
 application_name | wal_bytes |   %
------------------+-----------+-------
 app1             |  17708761 | 39.95
 app2             |  26614797 | 60.05
(2 rows)

3. Get the wal_bytes generated by database:

postgres=# SELECT datname, wal_bytes, round(100 * wal_bytes/sum(wal_bytes) over(),2) AS "%"
FROM  (SELECT datname, sum(wal_bytes) AS wal_bytes
       FROM pg_stat_activity, pg_stat_get_backend_wal(pid)
       WHERE wal_bytes != 0 GROUP BY datname);
 datname | wal_bytes |   %
---------+-----------+-------
 db1     |  35461858 | 80.01
 db2     |   8861700 | 19.99
(2 rows)

and much more...

=== Implementation ===

The same limitation as in 9aea73fc61 persists, meaning that Auxiliary processes
are not included in this set of statistics.

The patch is made of 3 sub-patches:

0001: to extract the logic filling pg_stat_get_wal()'s tuple into its own routine.
It adds pg_stat_wal_build_tuple(), a helper routine for pg_stat_get_wal(), that
fills its tuple based on the contents of PgStat_WalStats. Same idea as ff7c40d7fd.

0002: PGSTAT_KIND_BACKEND code refactoring. It refactors some come related to per
backend statistics. It makes the code more generic or more IO statistics focused
as it will be used in 0003 that will introduce per backend WAL statistics. It does
not add any new feature, that's 100% code refactoring to ease 0003 review.

0003: it adds the per backend WAL statistics and the new pg_stat_get_backend_wal()
function, documentation and related test. 

=== Remarks ===

R1:

0003 does not rely on pgstat_prep_backend_pending() for its pending statistics
but on a new PendingBackendWalStats variable. The reason is that the pending wal
statistics are incremented in a critical section (see XLogWrite(), and so
a call to pgstat_prep_pending_entry() could trigger a failed assertion:
MemoryContextAllocZero()->"CritSectionCount == 0 || (context)->allowInCritSection"

R2:

Instead of relying on a new PendingBackendWalStats, we could rely on the
existing PendingWalStats variable. But that would complicate the flush of
per backend and existing wal stats as that would need some coordination. I think
that it's better that each kind has its own pending variable.

R3:

Instead of incrementing the PendingBackendWalStats members individually we could
also "just" assign the PendingWalStats ones once incremented. I thought it's
better to make them "fully independent" though.

R4:

0002 introduces a new PgStat_BackendPending struct. Due to R1, that's not needed
per say but could have been if pgstat_prep_backend_pending() would have been
used. I keep this change as we may want to add more per backend stats in the future.

Looking forward to your feedback,

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  2. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-01-08T06:21:26Z

    On Tue, Jan 07, 2025 at 08:48:51AM +0000, Bertrand Drouvot wrote:
> Now that commit 9aea73fc61 added backend-level statistics to pgstats (and
> per backend IO statistics), we can more easily add per backend statistics.
> 
> Please find attached a patch to implement $SUBJECT.

I've looked at v1-0002 and v1-0001.

+static void
+pgstat_flush_io_entry(PgStat_EntryRef *entry_ref, bool nowait, bool need_lock)
 {
-	PgStatShared_Backend *shbackendioent;
-	PgStat_BackendPendingIO *pendingent;
+	PgStatShared_Backend *shbackendent;
+	PgStat_BackendPending *pendingent;
 	PgStat_BktypeIO *bktype_shstats;
+	PgStat_BackendPendingIO *pending_io;
 
-	if (!pgstat_lock_entry(entry_ref, nowait))
-		return false;
+	if (need_lock && !pgstat_lock_entry(entry_ref, nowait))
+		return;

The addition of need_lock at this level leads to a result that seems a
bit confusing, where pgstat_backend_flush_cb() passes "false" because
it locks the entry by itself as an effect of v1-0003 with the new area
for WAL.  Wouldn't it be cleaner to do an extra pgstat_[un]lock_entry
dance in pgstat_backend_flush_io() instead?  Another approach I can
think of that would be slightly cleaner to me is to pass a bits32 to a
single routine that would control if WAL stats, I/O stats or both
should be flushed, keeping pgstat_flush_backend() as name with an
extra argument to decide which parts of the stats should be flushed.

-PgStat_BackendPendingIO *
+PgStat_BackendPending *

This rename makes sense.

 #define PG_STAT_GET_WAL_COLS	9
 	TupleDesc	tupdesc;
 	Datum		values[PG_STAT_GET_WAL_COLS] = {0};
 	bool		nulls[PG_STAT_GET_WAL_COLS] = {0};

It feels unnatural to have a PG_STAT_GET_WAL_COLS while it would not
only relate to this function anymore.
--
Michael

  3. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-01-08T11:11:59Z

    Hi,

On Wed, Jan 08, 2025 at 03:21:26PM +0900, Michael Paquier wrote:
> On Tue, Jan 07, 2025 at 08:48:51AM +0000, Bertrand Drouvot wrote:
> > Now that commit 9aea73fc61 added backend-level statistics to pgstats (and
> > per backend IO statistics), we can more easily add per backend statistics.
> > 
> > Please find attached a patch to implement $SUBJECT.
> 
> I've looked at v1-0002 and v1-0001.

Thanks for looking at it!

> +static void
> +pgstat_flush_io_entry(PgStat_EntryRef *entry_ref, bool nowait, bool need_lock)
>  {
> -	PgStatShared_Backend *shbackendioent;
> -	PgStat_BackendPendingIO *pendingent;
> +	PgStatShared_Backend *shbackendent;
> +	PgStat_BackendPending *pendingent;
>  	PgStat_BktypeIO *bktype_shstats;
> +	PgStat_BackendPendingIO *pending_io;
>  
> -	if (!pgstat_lock_entry(entry_ref, nowait))
> -		return false;
> +	if (need_lock && !pgstat_lock_entry(entry_ref, nowait))
> +		return;
> 
> The addition of need_lock at this level leads to a result that seems a
> bit confusing, where pgstat_backend_flush_cb() passes "false" because
> it locks the entry by itself as an effect of v1-0003 with the new area
> for WAL.  Wouldn't it be cleaner to do an extra pgstat_[un]lock_entry
> dance in pgstat_backend_flush_io() instead?  Another approach I can
> think of that would be slightly cleaner to me is to pass a bits32 to a
> single routine that would control if WAL stats, I/O stats or both
> should be flushed, keeping pgstat_flush_backend() as name with an
> extra argument to decide which parts of the stats should be flushed.

Yeah, that's more elegant as it also means that the main callback will not change
(should we add even more stats in the future). Done that way in v2 attached.

> -PgStat_BackendPendingIO *
> +PgStat_BackendPending *
> 
> This rename makes sense.
> 
>  #define PG_STAT_GET_WAL_COLS	9
>  	TupleDesc	tupdesc;
>  	Datum		values[PG_STAT_GET_WAL_COLS] = {0};
>  	bool		nulls[PG_STAT_GET_WAL_COLS] = {0};
> 
> It feels unnatural to have a PG_STAT_GET_WAL_COLS while it would not
> only relate to this function anymore.

Has been renamed to PG_STAT_WAL_COLS in the attached.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  4. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-01-09T04:03:15Z

    On Wed, Jan 08, 2025 at 11:11:59AM +0000, Bertrand Drouvot wrote:
> Yeah, that's more elegant as it also means that the main callback will not change
> (should we add even more stats in the future). Done that way in v2 attached.

I've put my hands on v2-0002 to begin with something.

+/* flag bits for different types of statistics to flush */
+#define PGSTAT_FLUSH_IO    (1 << 0) /* Flush I/O statistics */
+#define PGSTAT_FLUSH_ALL   (PGSTAT_FLUSH_IO)

These are located and used only in pgstat_backend.c.  It seems to me
that we'd better declare them in pgstat_internal.h and extend the
existing pgstat_flush_backend() with an argument so as callers can do
what they want.

+	/* Get our own entry_ref if not provided */
+	if (!entry_ref)
+		entry_ref = pgstat_get_entry_ref(PGSTAT_KIND_BACKEND, InvalidOid,
+										 MyProcNumber, false, NULL);

This relates to the previous remark, actually, where I think that it
is cleaner to have pgstat_flush_backend() do pgstat_get_entry_ref(),
same way as HEAD, and just pass down the flags.  pgstat_flush_backend
cannot call directly pgstat_backend_flush_cb(), of course, so I've
settled down to a new pgstat_flush_backend_entry() that handles the
entry locking.  This comes at the cost of pgstat_flush_backend_entry()
requiring an extra pgstat_tracks_backend_bktype(), which is not a big
issue, and the patch gets a bit shorter.
--
Michael

  5. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-01-09T07:05:54Z

    Hi,

On Thu, Jan 09, 2025 at 01:03:15PM +0900, Michael Paquier wrote:
> On Wed, Jan 08, 2025 at 11:11:59AM +0000, Bertrand Drouvot wrote:
> > Yeah, that's more elegant as it also means that the main callback will not change
> > (should we add even more stats in the future). Done that way in v2 attached.
> 
> I've put my hands on v2-0002 to begin with something.
> 
> +/* flag bits for different types of statistics to flush */
> +#define PGSTAT_FLUSH_IO    (1 << 0) /* Flush I/O statistics */
> +#define PGSTAT_FLUSH_ALL   (PGSTAT_FLUSH_IO)
> 
> These are located and used only in pgstat_backend.c.  It seems to me
> that we'd better declare them in pgstat_internal.h and extend the
> existing pgstat_flush_backend() with an argument so as callers can do
> what they want.
> 
> +	/* Get our own entry_ref if not provided */
> +	if (!entry_ref)
> +		entry_ref = pgstat_get_entry_ref(PGSTAT_KIND_BACKEND, InvalidOid,
> +										 MyProcNumber, false, NULL);
> 
> This relates to the previous remark, actually, where I think that it
> is cleaner to have pgstat_flush_backend() do pgstat_get_entry_ref(),
> same way as HEAD, and just pass down the flags.

I see, so you keep pgstat_flush_backend() calls (with an extra arg) and remove
the new "pgstat_backend_flush_io()" function.

> This comes at the cost of pgstat_flush_backend_entry()
> requiring an extra pgstat_tracks_backend_bktype(), which is not a big
> issue, and the patch gets a bit shorter.

Yeah, all of the above is fine by me.

PFA v3 which is v2 refactoring with your proposed above changes.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  6. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-01-10T00:14:45Z

    On Thu, Jan 09, 2025 at 07:05:54AM +0000, Bertrand Drouvot wrote:
> PFA v3 which is v2 refactoring with your proposed above changes.

An extra thing I have finished by doing is removing
PgStat_BackendPendingIO, then applied the change.  It was useful when
returned as a result of pgstat_prep_backend_pending(), but not so much
with the new PgStat_BackendPending that includes all the pending stats
data.
--
Michael

  7. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-01-10T09:40:38Z

    Hi,

Michael Paquier wrote:
> An extra thing I have finished by doing is removing
> PgStat_BackendPendingIO, then applied the change.  It was useful when
> returned as a result of pgstat_prep_backend_pending(), but not so much
> with the new PgStat_BackendPending that includes all the pending stats
> data.

Yeah, makes sense, thanks!

Please find attached v4 taking into account 2c14037bb5.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  8. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-01-15T06:11:32Z

    On Fri, Jan 10, 2025 at 09:40:38AM +0000, Bertrand Drouvot wrote:
> Please find attached v4 taking into account 2c14037bb5.

+} PgStat_WalCounters;
+
+typedef struct PgStat_WalStats
+{
+	PgStat_WalCounters wal_counters;

I know that's a nit, but perhaps that could be a patch of its own,
pushing that to pg_stat_wal_build_tuple() to reduce the diffs in the
main patch.

- * Find or create a local PgStat_BackendPending entry for proc number.
+ * Find or create a local PgStat_BackendPendingIO entry for proc number.

Seems like you are undoing a change here.

+ * WAL pending statistics are incremented inside a critical section
+ * (see XLogWrite()), so we can't use pgstat_prep_pending_entry() and we rely on
+ * PendingBackendWalStats instead.
+ */
+extern PGDLLIMPORT PgStat_PendingWalStats PendingBackendWalStats;

Hmm.  This makes me wonder if we should rethink a bit the way pending
entries are retrieved and if we should do it beforehand for the WAL
paths to avoid allocations in some critical sections.  Isn't that also
because we avoid calling pgstat_prep_backend_pending() for the I/O
case as only backends are supported now, discarding cases like the
checkpointer where I/O could happen in a critical path?  As a whole,
the approach taken by the patch is not really consistent with the
rest.
--
Michael

  9. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-01-16T15:59:31Z

    Hi,

On Wed, Jan 15, 2025 at 03:11:32PM +0900, Michael Paquier wrote:
> On Fri, Jan 10, 2025 at 09:40:38AM +0000, Bertrand Drouvot wrote:
> > Please find attached v4 taking into account 2c14037bb5.
> 
> +} PgStat_WalCounters;
> +
> +typedef struct PgStat_WalStats
> +{
> +	PgStat_WalCounters wal_counters;
> 
> I know that's a nit, but perhaps that could be a patch of its own,
> pushing that to pg_stat_wal_build_tuple() to reduce the diffs in the
> main patch.

Done in 0003 attached.

> - * Find or create a local PgStat_BackendPending entry for proc number.
> + * Find or create a local PgStat_BackendPendingIO entry for proc number.
> 
> Seems like you are undoing a change here.

Arf, nice catch, hopefully that's only the comment.. Fixed in the attached.

> + * WAL pending statistics are incremented inside a critical section
> + * (see XLogWrite()), so we can't use pgstat_prep_pending_entry() and we rely on
> + * PendingBackendWalStats instead.
> + */
> +extern PGDLLIMPORT PgStat_PendingWalStats PendingBackendWalStats;
> 
> Hmm.  This makes me wonder if we should rethink a bit the way pending
> entries are retrieved and if we should do it beforehand for the WAL
> paths to avoid allocations in some critical sections.  Isn't that also
> because we avoid calling pgstat_prep_backend_pending() for the I/O
> case as only backends are supported now, discarding cases like the
> checkpointer where I/O could happen in a critical path?  As a whole,
> the approach taken by the patch is not really consistent with the
> rest.

I agree that's better to have a generic solution and to be consistent with
the other variable-numbered stats. 

The attached is implementing in 0001 the proposition done in [1], i.e:

1. It adds a new allow_critical_section to PgStat_KindInfo for pgstats kinds
2. It ensures to set temporarly allowInCritSection to true when needed

Note that for safety reason 0001 does set allowInCritSection back to false
unconditionally (means not checking again for allow_critical_section).

While it avoids the failed assertion mentioned above and in [1] (on
the MemoryContextAllocZero() call), the TAP tests are still failing with a new
failed assertion.

If you apply the whole patch series attached, you'll see that:

make -C src/bin/pg_rewind check PROVE_TESTS=t/001_basic.pl

is failing with something like:

TRAP: failed Assert("CritSectionCount == 0"), File: "mcxt.c", Line: 1107, PID: 3295726
pg18/bin/postgres(ExceptionalCondition+0xbb)[0x59668bee1f6d]
pg18/bin/postgres(MemoryContextCreate+0x46)[0x59668bf2a8fe]
pg18/bin/postgres(AllocSetContextCreateInternal+0x1df)[0x59668bf1bb11]
pg18/bin/postgres(pgstat_prep_pending_entry+0x86)[0x59668bcff8cc]
pg18/bin/postgres(pgstat_prep_backend_pending+0x2b)[0x59668bd024a9]

This one is more problematic because we are in MemoryContextCreate() so that the
"workaround" above does not help. I need to put more thoughts on it but already
sharing the issue here (as also discussed in [1]).

[1]: https://www.postgresql.org/message-id/Z4d_eggsxtBEdJAG%40paquier.xyz

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  10. Re: per backend WAL statistics

    Andres Freund <andres@anarazel.de> — 2025-01-16T16:38:47Z

    Hi,

On 2025-01-16 15:59:31 +0000, Bertrand Drouvot wrote:
> On Wed, Jan 15, 2025 at 03:11:32PM +0900, Michael Paquier wrote:
> > On Fri, Jan 10, 2025 at 09:40:38AM +0000, Bertrand Drouvot wrote:
> > + * WAL pending statistics are incremented inside a critical section
> > + * (see XLogWrite()), so we can't use pgstat_prep_pending_entry() and we rely on
> > + * PendingBackendWalStats instead.
> > + */
> > +extern PGDLLIMPORT PgStat_PendingWalStats PendingBackendWalStats;
> > 
> > Hmm.  This makes me wonder if we should rethink a bit the way pending
> > entries are retrieved and if we should do it beforehand for the WAL
> > paths to avoid allocations in some critical sections.  Isn't that also
> > because we avoid calling pgstat_prep_backend_pending() for the I/O
> > case as only backends are supported now, discarding cases like the
> > checkpointer where I/O could happen in a critical path?  As a whole,
> > the approach taken by the patch is not really consistent with the
> > rest.

> I agree that's better to have a generic solution and to be consistent with
> the other variable-numbered stats. 
> 
> The attached is implementing in 0001 the proposition done in [1], i.e:
> 
> 1. It adds a new allow_critical_section to PgStat_KindInfo for pgstats kinds
> 2. It ensures to set temporarly allowInCritSection to true when needed
>
> Note that for safety reason 0001 does set allowInCritSection back to false
> unconditionally (means not checking again for allow_critical_section).

This is a preposterously bad idea.  The restriction to not allocate memory in
critical sections exists for a reason, why on earth should this code be
allowed to just opt out of the restriction of not allowing memory allocations
in critical sections?

The only cases where we can somewhat safely allocate memory in critical
sections is when using memory contexts with pre-reserved memory, where there's
a pretty low bound on how much memory is going to be needed. E.g. logging a
message inside a critical section, where elog.c can reset ErrorContext
afterwards.

Greetings,

Andres Freund

  11. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-01-16T17:11:09Z

    Hi,

On Thu, Jan 16, 2025 at 11:38:47AM -0500, Andres Freund wrote:
> Hi,
> 
> On 2025-01-16 15:59:31 +0000, Bertrand Drouvot wrote:
> > On Wed, Jan 15, 2025 at 03:11:32PM +0900, Michael Paquier wrote:
> > > On Fri, Jan 10, 2025 at 09:40:38AM +0000, Bertrand Drouvot wrote:
> > > + * WAL pending statistics are incremented inside a critical section
> > > + * (see XLogWrite()), so we can't use pgstat_prep_pending_entry() and we rely on
> > > + * PendingBackendWalStats instead.
> > > + */
> > > +extern PGDLLIMPORT PgStat_PendingWalStats PendingBackendWalStats;
> > > 
> > > Hmm.  This makes me wonder if we should rethink a bit the way pending
> > > entries are retrieved and if we should do it beforehand for the WAL
> > > paths to avoid allocations in some critical sections.  Isn't that also
> > > because we avoid calling pgstat_prep_backend_pending() for the I/O
> > > case as only backends are supported now, discarding cases like the
> > > checkpointer where I/O could happen in a critical path?  As a whole,
> > > the approach taken by the patch is not really consistent with the
> > > rest.
> 
> > I agree that's better to have a generic solution and to be consistent with
> > the other variable-numbered stats. 
> > 
> > The attached is implementing in 0001 the proposition done in [1], i.e:
> > 
> > 1. It adds a new allow_critical_section to PgStat_KindInfo for pgstats kinds
> > 2. It ensures to set temporarly allowInCritSection to true when needed
> >
> > Note that for safety reason 0001 does set allowInCritSection back to false
> > unconditionally (means not checking again for allow_critical_section).
> 
> This is a preposterously bad idea.  The restriction to not allocate memory in
> critical sections exists for a reason,

Thanks for sharing your thoughts on it. In [1], you said:

"
My view is that for IO stats no memory allocation should be required - that
used to be the case and should be the case again
"

So, do you think that the initial proposal that has been made here (See R1. in
[2]) i.e make use of a new PendingBackendWalStats variable:

"
0003 does not rely on pgstat_prep_backend_pending() for its pending statistics
but on a new PendingBackendWalStats variable. The reason is that the pending wal
statistics are incremented in a critical section (see XLogWrite(), and so
a call to pgstat_prep_pending_entry() could trigger a failed assertion:
MemoryContextAllocZero()->"CritSectionCount == 0 || (context)->allowInCritSection"
"

and implemented up to v4 is a viable approach?

[1]: https://www.postgresql.org/message-id/66efowskppsns35v5u2m7k4sdnl7yoz5bo64tdjwq7r5lhplrz%40y7dme5xwh2r5
[2]: https://www.postgresql.org/message-id/Z3zqc4o09dM/Ezyz%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  12. Re: per backend WAL statistics

    Andres Freund <andres@anarazel.de> — 2025-01-16T17:44:20Z

    Hi,

On 2025-01-16 17:11:09 +0000, Bertrand Drouvot wrote:
> On Thu, Jan 16, 2025 at 11:38:47AM -0500, Andres Freund wrote:
> > Hi,
> > 
> > On 2025-01-16 15:59:31 +0000, Bertrand Drouvot wrote:
> > > On Wed, Jan 15, 2025 at 03:11:32PM +0900, Michael Paquier wrote:
> > > > On Fri, Jan 10, 2025 at 09:40:38AM +0000, Bertrand Drouvot wrote:
> > > > + * WAL pending statistics are incremented inside a critical section
> > > > + * (see XLogWrite()), so we can't use pgstat_prep_pending_entry() and we rely on
> > > > + * PendingBackendWalStats instead.
> > > > + */
> > > > +extern PGDLLIMPORT PgStat_PendingWalStats PendingBackendWalStats;
> > > > 
> > > > Hmm.  This makes me wonder if we should rethink a bit the way pending
> > > > entries are retrieved and if we should do it beforehand for the WAL
> > > > paths to avoid allocations in some critical sections.  Isn't that also
> > > > because we avoid calling pgstat_prep_backend_pending() for the I/O
> > > > case as only backends are supported now, discarding cases like the
> > > > checkpointer where I/O could happen in a critical path?  As a whole,
> > > > the approach taken by the patch is not really consistent with the
> > > > rest.
> > 
> > > I agree that's better to have a generic solution and to be consistent with
> > > the other variable-numbered stats. 
> > > 
> > > The attached is implementing in 0001 the proposition done in [1], i.e:
> > > 
> > > 1. It adds a new allow_critical_section to PgStat_KindInfo for pgstats kinds
> > > 2. It ensures to set temporarly allowInCritSection to true when needed
> > >
> > > Note that for safety reason 0001 does set allowInCritSection back to false
> > > unconditionally (means not checking again for allow_critical_section).
> > 
> > This is a preposterously bad idea.  The restriction to not allocate memory in
> > critical sections exists for a reason,
> 
> Thanks for sharing your thoughts on it. In [1], you said:
> 
> "
> My view is that for IO stats no memory allocation should be required - that
> used to be the case and should be the case again
> "
> 
> So, do you think that the initial proposal that has been made here (See R1. in
> [2]) i.e make use of a new PendingBackendWalStats variable:

Well, I think this first needs be fixed for for the IO stats change made in

commit 9aea73fc61d
Author: Michael Paquier <michael@paquier.xyz>
Date:   2024-12-19 13:19:22 +0900
 
    Add backend-level statistics to pgstats


Once we have a pattern to model after, we can apply the same scheme here.


> "
> 0003 does not rely on pgstat_prep_backend_pending() for its pending statistics
> but on a new PendingBackendWalStats variable. The reason is that the pending wal
> statistics are incremented in a critical section (see XLogWrite(), and so
> a call to pgstat_prep_pending_entry() could trigger a failed assertion:
> MemoryContextAllocZero()->"CritSectionCount == 0 || (context)->allowInCritSection"
> "
> 
> and implemented up to v4 is a viable approach?

Yes-ish.  I think it would be better to make it slightly more general than
that, handling this for all types of backend stats, not just for WAL.

Greetings,

Andres Freund

  13. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-01-16T23:43:57Z

    On Thu, Jan 16, 2025 at 12:44:20PM -0500, Andres Freund wrote:
> On 2025-01-16 17:11:09 +0000, Bertrand Drouvot wrote:
>> So, do you think that the initial proposal that has been made here (See R1. in
>> [2]) i.e make use of a new PendingBackendWalStats variable:
> 
> Well, I think this first needs be fixed for for the IO stats change made in
> 
> Once we have a pattern to model after, we can apply the same scheme here.

Okay, thanks for the input.  I was not sure what you intended
originally with all this part of the backend code, and how much would
be acceptable.  The line is clear now.

>> 0003 does not rely on pgstat_prep_backend_pending() for its pending statistics
>> but on a new PendingBackendWalStats variable. The reason is that the pending wal
>> statistics are incremented in a critical section (see XLogWrite(), and so
>> a call to pgstat_prep_pending_entry() could trigger a failed assertion:
>> MemoryContextAllocZero()->"CritSectionCount == 0 || (context)->allowInCritSection"
>> "
>> 
>> and implemented up to v4 is a viable approach?
> 
> Yes-ish.  I think it would be better to make it slightly more general than
> that, handling this for all types of backend stats, not just for WAL.

Agreed to use the same concept for all these parts of the backend
stats kind rather than two of them.  Will send a reply on the original
backend I/O thread as well.
--
Michael

  14. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-01-21T07:19:55Z

    Hi,

On Fri, Jan 17, 2025 at 08:43:57AM +0900, Michael Paquier wrote:
> On Thu, Jan 16, 2025 at 12:44:20PM -0500, Andres Freund wrote:
> > On 2025-01-16 17:11:09 +0000, Bertrand Drouvot wrote:
> >> So, do you think that the initial proposal that has been made here (See R1. in
> >> [2]) i.e make use of a new PendingBackendWalStats variable:
> > 
> > Well, I think this first needs be fixed for for the IO stats change made in
> > 
> > Once we have a pattern to model after, we can apply the same scheme here.
> 
> Okay, thanks for the input.  I was not sure what you intended
> originally with all this part of the backend code, and how much would
> be acceptable.  The line is clear now.
> 
> >> 0003 does not rely on pgstat_prep_backend_pending() for its pending statistics
> >> but on a new PendingBackendWalStats variable. The reason is that the pending wal
> >> statistics are incremented in a critical section (see XLogWrite(), and so
> >> a call to pgstat_prep_pending_entry() could trigger a failed assertion:
> >> MemoryContextAllocZero()->"CritSectionCount == 0 || (context)->allowInCritSection"
> >> "
> >> 
> >> and implemented up to v4 is a viable approach?
> > 
> > Yes-ish.  I think it would be better to make it slightly more general than
> > that, handling this for all types of backend stats, not just for WAL.
> 
> Agreed to use the same concept for all these parts of the backend
> stats kind rather than two of them.  Will send a reply on the original
> backend I/O thread as well.

PFA v6 that now relies on the new PendingBackendStats variable introduced in
4feba03d8b9.

Remark: I moved PendingBackendStats back to pgstat.h because I think that the
"simple" pending stats increment that we are adding in xlog.c are not worth
an extra function call overhead (while it made more sense for the more complex IO
stats handling). So PendingBackendStats is now visible to the outside world like
PendingWalStats and friends.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  15. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-01-23T08:05:30Z

    On Tue, Jan 21, 2025 at 07:19:55AM +0000, Bertrand Drouvot wrote:
> PFA v6 that now relies on the new PendingBackendStats variable introduced in
> 4feba03d8b9.
> 
> Remark: I moved PendingBackendStats back to pgstat.h because I think that the
> "simple" pending stats increment that we are adding in xlog.c are not worth
> an extra function call overhead (while it made more sense for the more complex IO
> stats handling). So PendingBackendStats is now visible to the outside world like
> PendingWalStats and friends.

You are re-doing here a pattern I was trying to avoid so as we don't
copy-paste more checks based on pgstat_tracks_backend_bktype more than
necessary.  I am wondering if we should think harder about the
interface used to register WAL stats, and make it more consistent with
the way pg_stat_io is handled, avoiding the hardcoded attribute
numbers if we have an enum to control which field to update in some
input routine.

As we have only five counters in PgStat_PendingWalStats, the result
you have is not that invasive, true.

Are you sure that the interactions between pgWalUsage, prevWalUsage
and prevBackendWalUsage are correct?  As far I got it from a code
read, prevWalUsage, prevBackendWalUsage and their local trackings in
pgstat_backend.c and pgstat_wal.c rely on instrument.c as the primary
source, as pgWalUsage can never be reset.  Is that right?
--
Michael

  16. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-01-23T09:57:50Z

    Hi,

On Thu, Jan 23, 2025 at 05:05:30PM +0900, Michael Paquier wrote:
> On Tue, Jan 21, 2025 at 07:19:55AM +0000, Bertrand Drouvot wrote:
> > PFA v6 that now relies on the new PendingBackendStats variable introduced in
> > 4feba03d8b9.
> > 
> > Remark: I moved PendingBackendStats back to pgstat.h because I think that the
> > "simple" pending stats increment that we are adding in xlog.c are not worth
> > an extra function call overhead (while it made more sense for the more complex IO
> > stats handling). So PendingBackendStats is now visible to the outside world like
> > PendingWalStats and friends.
> 
> You are re-doing here a pattern I was trying to avoid so as we don't
> copy-paste more checks based on pgstat_tracks_backend_bktype more than
> necessary.

I'm not sure I get it. pgstat_tracks_backend_bktype() is also called in
pgstat_count_backend_io_op() and pgstat_count_backend_io_op_time(). What issue
do you see with the extra calls part of this patch?

> I am wondering if we should think harder about the
> interface used to register WAL stats, and make it more consistent with
> the way pg_stat_io is handled, avoiding the hardcoded attribute
> numbers if we have an enum to control which field to update in some
> input routine.

Not sure as WAL stats just tracks a single dimension unlike IO stats which track
both IOObject and IOContext. What would be the benefit(s)?

> As we have only five counters in PgStat_PendingWalStats, the result
> you have is not that invasive, true.

And only one dimension.

> Are you sure that the interactions between pgWalUsage, prevWalUsage
> and prevBackendWalUsage are correct?

I think so and according to my testing I can see WalUsage values
that correlate nicely between pg_stat_wal() and pg_stat_get_backend_wal().

> As far I got it from a code
> read, prevWalUsage, prevBackendWalUsage and their local trackings in
> pgstat_backend.c and pgstat_wal.c rely on instrument.c as the primary
> source, as pgWalUsage can never be reset.  Is that right?

yeah, IIUC pgWalUsage acts as the primary source that both prevWalUsage and
prevBackendWalUsage diff against to calculate incremental stats.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  17. Re: per backend WAL statistics

    Rahila Syed <rahilasyed90@gmail.com> — 2025-01-29T07:44:09Z

    Hi,

Thank you for the patchset. Having per-backend WAL statistics,
in addition to cluster-wide ones, is useful.

I had a few comments while looking at v6-0003-* patch.

+ /*
+ * This could be an auxiliary process but these do not report backend
+ * statistics due to pgstat_tracks_backend_bktype(), so there is no need
+ * for an extra call to AuxiliaryPidGetProc().
+ */
+ if (!proc)
+ PG_RETURN_NULL();

Maybe an explicit call to AuxiliaryPidGetProc() followed by a check
for pgstat_tracks_backend_bktype() would be more maintainable.
Since the processes tracked by AuxiliaryPidGetProc and
pgstat_tracks_backend_bktype might diverge in future.

On that note, it is not clear to me why the WAL writer statistics are not
included in per backend
wal statistics? I understand the same limitation currently exists in
pgstats_track_io_bktype(), but why does that need to be extended to
WAL statistics?

+         <primary>pg_stat_get_backend_wal</primary>
+        </indexterm>
+        <function>pg_stat_get_backend_wal</function> (
<type>integer</type> )
+        <returnvalue>record</returnvalue>
+       </para>
Should the naming describe what is being returned more clearly?
Something like pg_stat_get_backend_wal_activity()? Currently it
suggests that it returns a backend's WAL, which is not the case.

+ if (pgstat_tracks_backend_bktype(MyBackendType))
+ {
+ PendingBackendStats.pending_wal.wal_write++;
+
+ if (track_wal_io_timing)
+ INSTR_TIME_ACCUM_DIFF(PendingBackendStats.pending_wal.wal_write_time,
+  end, start);
+ }
At the risk of nitpicking, may I suggest moving the above code, which is
under the
track_wal_io_timing check, to the existing check before this added chunk?
This way, all code related to track_wal_io_timing will be grouped together,
closer to where the "end" variable is computed.


Thank you,
Rahila Syed



On Tue, Jan 21, 2025 at 12:50 PM Bertrand Drouvot <
bertranddrouvot.pg@gmail.com> wrote:

> Hi,
>
> On Fri, Jan 17, 2025 at 08:43:57AM +0900, Michael Paquier wrote:
> > On Thu, Jan 16, 2025 at 12:44:20PM -0500, Andres Freund wrote:
> > > On 2025-01-16 17:11:09 +0000, Bertrand Drouvot wrote:
> > >> So, do you think that the initial proposal that has been made here
> (See R1. in
> > >> [2]) i.e make use of a new PendingBackendWalStats variable:
> > >
> > > Well, I think this first needs be fixed for for the IO stats change
> made in
> > >
> > > Once we have a pattern to model after, we can apply the same scheme
> here.
> >
> > Okay, thanks for the input.  I was not sure what you intended
> > originally with all this part of the backend code, and how much would
> > be acceptable.  The line is clear now.
> >
> > >> 0003 does not rely on pgstat_prep_backend_pending() for its pending
> statistics
> > >> but on a new PendingBackendWalStats variable. The reason is that the
> pending wal
> > >> statistics are incremented in a critical section (see XLogWrite(),
> and so
> > >> a call to pgstat_prep_pending_entry() could trigger a failed
> assertion:
> > >> MemoryContextAllocZero()->"CritSectionCount == 0 ||
> (context)->allowInCritSection"
> > >> "
> > >>
> > >> and implemented up to v4 is a viable approach?
> > >
> > > Yes-ish.  I think it would be better to make it slightly more general
> than
> > > that, handling this for all types of backend stats, not just for WAL.
> >
> > Agreed to use the same concept for all these parts of the backend
> > stats kind rather than two of them.  Will send a reply on the original
> > backend I/O thread as well.
>
> PFA v6 that now relies on the new PendingBackendStats variable introduced
> in
> 4feba03d8b9.
>
> Remark: I moved PendingBackendStats back to pgstat.h because I think that
> the
> "simple" pending stats increment that we are adding in xlog.c are not worth
> an extra function call overhead (while it made more sense for the more
> complex IO
> stats handling). So PendingBackendStats is now visible to the outside
> world like
> PendingWalStats and friends.
>
> Regards,
>
> --
> Bertrand Drouvot
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com
>

  18. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-01-29T13:32:37Z

    Hi,

On Wed, Jan 29, 2025 at 01:14:09PM +0530, Rahila Syed wrote:
> Hi,
> 
> Thank you for the patchset. Having per-backend WAL statistics,
> in addition to cluster-wide ones, is useful.

Thanks for looking at it!

> I had a few comments while looking at v6-0003-* patch.
> 
> + /*
> + * This could be an auxiliary process but these do not report backend
> + * statistics due to pgstat_tracks_backend_bktype(), so there is no need
> + * for an extra call to AuxiliaryPidGetProc().
> + */
> + if (!proc)
> + PG_RETURN_NULL();
> 
> Maybe an explicit call to AuxiliaryPidGetProc() followed by a check
> for pgstat_tracks_backend_bktype() would be more maintainable.
> Since the processes tracked by AuxiliaryPidGetProc and
> pgstat_tracks_backend_bktype might diverge in future.

I think that could make sense but that might need a separate thread as this
is not only related to this patch (already done that way in pg_stat_reset_backend_stats()
and pg_stat_get_backend_io()).

> On that note, it is not clear to me why the WAL writer statistics are not
> included in per backend
> wal statistics? I understand the same limitation currently exists in
> pgstats_track_io_bktype(), but why does that need to be extended to
> WAL statistics?

WAL writer might be fine but that would not add that much value here because
it's going to appear anyway in pg_stat_io once Nazir's patch [1] gets in.

> +         <primary>pg_stat_get_backend_wal</primary>
> +        </indexterm>
> +        <function>pg_stat_get_backend_wal</function> (
> <type>integer</type> )
> +        <returnvalue>record</returnvalue>
> +       </para>
> Should the naming describe what is being returned more clearly?
> Something like pg_stat_get_backend_wal_activity()? Currently it
> suggests that it returns a backend's WAL, which is not the case.

Not sure. It aligns with pg_stat_get_backend_io() and the "stat" in its name
suggests this is related to stats.

> + if (pgstat_tracks_backend_bktype(MyBackendType))
> + {
> + PendingBackendStats.pending_wal.wal_write++;
> +
> + if (track_wal_io_timing)
> + INSTR_TIME_ACCUM_DIFF(PendingBackendStats.pending_wal.wal_write_time,
> +  end, start);
> + }
> At the risk of nitpicking, may I suggest moving the above code, which is
> under the
> track_wal_io_timing check, to the existing check before this added chunk?
> This way, all code related to track_wal_io_timing will be grouped together,
> closer to where the "end" variable is computed.

I think we're waiting [1] to be in before moving forward with this patch. I think
that [1] also touches this part of the code. I'll keep your remark in mind and
see if it still makes sense once [1] gets in.

[1]: https://www.postgresql.org/message-id/flat/CAN55FZ3AiQ%2BZMxUuXnBpd0Rrh1YhwJ5FudkHg%3DJU0P%2B-W8T4Vg%40mail.gmail.com

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  19. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-02-04T08:49:41Z

    Hi,

On Thu, Jan 23, 2025 at 09:57:50AM +0000, Bertrand Drouvot wrote:
> On Thu, Jan 23, 2025 at 05:05:30PM +0900, Michael Paquier wrote:
> > As far I got it from a code
> > read, prevWalUsage, prevBackendWalUsage and their local trackings in
> > pgstat_backend.c and pgstat_wal.c rely on instrument.c as the primary
> > source, as pgWalUsage can never be reset.  Is that right?
> 
> yeah, IIUC pgWalUsage acts as the primary source that both prevWalUsage and
> prevBackendWalUsage diff against to calculate incremental stats.
> 

Now that a051e71e28a is in, I think that we can reduce the scope of this patch
(i.e reduce the number of stats provided by pg_stat_get_backend_wal()).

I think we can keep:

wal_records
wal_fpi 
wal_bytes (because it differs from write_bytes in pg_stat_get_backend_io())
wal_buffers_full

The first 3 are in the WalUsage struct.

I think that: 

wal_write (and wal_write_time)
wal_sync (and wal_sync_time)

can be extracted from pg_stat_get_backend_io(), so there is no need to duplicate
this information. The same comment could be done for pg_stat_wal and pg_stat_io
though, but pg_stat_wal already exists so removing fields has not the same
effect.

What are you thoughts about keeping in pg_stat_get_backend_wal() only the
4 stats mentioned above?

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  20. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-02-05T02:16:15Z

    On Tue, Feb 04, 2025 at 08:49:41AM +0000, Bertrand Drouvot wrote:
> I think that: 
> 
> wal_write (and wal_write_time)
> wal_sync (and wal_sync_time)

Right.  We are not able to get this data from XLogWrite() and
issue_xlog_fsync(), so there is no need to duplicate that anymore in
your patch.

> can be extracted from pg_stat_get_backend_io(), so there is no need to duplicate
> this information. The same comment could be done for pg_stat_wal and pg_stat_io
> though, but pg_stat_wal already exists so removing fields has not the same
> effect.
> 
> What are you thoughts about keeping in pg_stat_get_backend_wal() only the
> 4 stats mentioned above?

wal_buffers_full is incremented in AdvanceXLInsertBuffer(), part of
PendingWalStats.  wal_records, wal_fpi and wal_bytes are part of the
instrumentation field.  It looks to me that if you discard the
wal_buffers_full part, the implementation of the data in the backend
could just be tied to the fields coming from WalUsage.

Actually, could it actually be useful to have wal_buffers_full be
available in WalUsage, so as it would show up in EXPLAIN in a
per-query basis with show_wal_usage()?  Consolidating that would make
what you are trying it a bit easier, because we would have the
WalUsage and the pg_stat_io parts without any of the PendingWalStats
part.  And it is just a counter not that expensive to handle, like the
data for records, fpis and bytes.  This extra information could be
useful to have in the context of an EXPLAIN.
--
Michael

  21. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-02-05T10:22:55Z

    Hi,

On Wed, Feb 05, 2025 at 11:16:15AM +0900, Michael Paquier wrote:
> On Tue, Feb 04, 2025 at 08:49:41AM +0000, Bertrand Drouvot wrote:
> > can be extracted from pg_stat_get_backend_io(), so there is no need to duplicate
> > this information. The same comment could be done for pg_stat_wal and pg_stat_io
> > though, but pg_stat_wal already exists so removing fields has not the same
> > effect.
> > 
> > What are you thoughts about keeping in pg_stat_get_backend_wal() only the
> > 4 stats mentioned above?
> 
> wal_buffers_full is incremented in AdvanceXLInsertBuffer(), part of
> PendingWalStats.  wal_records, wal_fpi and wal_bytes are part of the
> instrumentation field.  It looks to me that if you discard the
> wal_buffers_full part, the implementation of the data in the backend
> could just be tied to the fields coming from WalUsage.

Yup.

> Actually, could it actually be useful to have wal_buffers_full be
> available in WalUsage, so as it would show up in EXPLAIN in a
> per-query basis with show_wal_usage()?

Yeah, that might help. One could not be 100% sure that the statement being
explained is fully responsible of the wal buffer being full (as it could just be
a "victim" of an already almost full wal buffer). But OTOH that might help to
understand why an EXPLAIN analyze is slower than another one (i.e one generating
wal buffer full and the other not). Also I think it could be added to
pg_stat_statements and could also provide valuable information.

> Consolidating that would make
> what you are trying it a bit easier, because we would have the
> WalUsage and the pg_stat_io parts without any of the PendingWalStats
> part.  And it is just a counter not that expensive to handle, like the
> data for records, fpis and bytes.  This extra information could be
> useful to have in the context of an EXPLAIN.

Yeah, I did a bit of archeology to try to understand why it's not already the
case. From what I can see, in commit time order:

1. df3b181499 introduced the WalUsage structure
2. 6b466bf5f2 added the wal usage in pg_stat_statements
3. 33e05f89c5 added the wal usage in EXPLAIN
4. 8d9a935965f added pg_stat_wal (and wal_buffers_full)
5. 01469241b2f added the wal usage in pg_stat_wal 

So, wal_buffers_full has been introduced after the WalUsage structure was
there but I don't see any reason in the emails as to why it's not in the WalUsage
structure (I might have missed it though).

I think that this proposal makes sense but would need a dedicated thread,
thoughts?

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  22. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-02-05T10:31:13Z

    On Wed, Feb 05, 2025 at 10:22:55AM +0000, Bertrand Drouvot wrote:
> So, wal_buffers_full has been introduced after the WalUsage structure was
> there but I don't see any reason in the emails as to why it's not in the WalUsage
> structure (I might have missed it though).
> 
> I think that this proposal makes sense but would need a dedicated thread,
> thoughts?

Using a separate thread for a change like that makes sense to me.  I
have to admit that the simplifications in terms of designs for what
we're discussing here makes such a change more valuable.  Adding this
information to WalUsage is one thing.  Showing it in EXPLAIN is a
second thing.  Doing the former simplifies the patch you are proposing
here.  We don't necessarily have to do the latter, but I don't see a
reason to not do it, either.
--
Michael

  23. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-02-05T14:28:08Z

    Hi,

On Wed, Feb 05, 2025 at 07:31:13PM +0900, Michael Paquier wrote:
> On Wed, Feb 05, 2025 at 10:22:55AM +0000, Bertrand Drouvot wrote:
> > So, wal_buffers_full has been introduced after the WalUsage structure was
> > there but I don't see any reason in the emails as to why it's not in the WalUsage
> > structure (I might have missed it though).
> > 
> > I think that this proposal makes sense but would need a dedicated thread,
> > thoughts?
> 
> Using a separate thread for a change like that makes sense to me.  I
> have to admit that the simplifications in terms of designs for what
> we're discussing here makes such a change more valuable.  Adding this
> information to WalUsage is one thing.  Showing it in EXPLAIN is a
> second thing.  Doing the former simplifies the patch you are proposing
> here.  We don't necessarily have to do the latter, but I don't see a
> reason to not do it, either.

Agree, I'll start a dedicated thread for that.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  24. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-02-06T01:38:55Z

    On Wed, Feb 05, 2025 at 02:28:08PM +0000, Bertrand Drouvot wrote:
> Agree, I'll start a dedicated thread for that.

Thanks.
--
Michael

  25. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-02-06T10:28:48Z

    Hi,

On Thu, Feb 06, 2025 at 10:38:55AM +0900, Michael Paquier wrote:
> On Wed, Feb 05, 2025 at 02:28:08PM +0000, Bertrand Drouvot wrote:
> > Agree, I'll start a dedicated thread for that.
> 
> Thanks.

Done in [1].

[1]: https://www.postgresql.org/message-id/flat/Z6SOha5YFFgvpwQY%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  26. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-02-17T06:59:40Z

    Hi,

On Thu, Feb 06, 2025 at 10:28:48AM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Thu, Feb 06, 2025 at 10:38:55AM +0900, Michael Paquier wrote:
> > On Wed, Feb 05, 2025 at 02:28:08PM +0000, Bertrand Drouvot wrote:
> > > Agree, I'll start a dedicated thread for that.
> > 
> > Thanks.
> 
> Done in [1].
> 
> [1]: https://www.postgresql.org/message-id/flat/Z6SOha5YFFgvpwQY%40ip-10-97-1-34.eu-west-3.compute.internal

Thanks for having committed the work done in [1] above.

There is still something that would simplify what is done here: it's the
"the elimination of the write & sync columns for pg_stat_wal" mentioned in [2].

I'll add it in the new patch serie for this thread (that simplifies the new 
pg_stat_wal_build_tuple() among other things) unless Nazir beat me to it.

[2]: https://www.postgresql.org/message-id/Z6L3ZNGCljZZouvN%40paquier.xyz

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  27. Re: per backend WAL statistics

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2025-02-17T07:08:43Z

    Hi,

Thank you for working on this!

On Mon, 17 Feb 2025 at 09:59, Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> There is still something that would simplify what is done here: it's the
> "the elimination of the write & sync columns for pg_stat_wal" mentioned in [2].
>
> I'll add it in the new patch serie for this thread (that simplifies the new
> pg_stat_wal_build_tuple() among other things) unless Nazir beat me to it.
>
> [2]: https://www.postgresql.org/message-id/Z6L3ZNGCljZZouvN%40paquier.xyz

I am working on some other stuff right now. Please feel free to work on it.

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

  28. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-02-17T07:25:46Z

    On Mon, Feb 17, 2025 at 06:59:40AM +0000, Bertrand Drouvot wrote:
> There is still something that would simplify what is done here: it's the
> "the elimination of the write & sync columns for pg_stat_wal" mentioned in [2].

Yeah, still you cannot just remove them because the data tracked in
pg_stat_io is not entirely the same, right?
--
Michael

  29. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-02-17T07:59:26Z

    Hi,

On Mon, Feb 17, 2025 at 04:25:46PM +0900, Michael Paquier wrote:
> On Mon, Feb 17, 2025 at 06:59:40AM +0000, Bertrand Drouvot wrote:
> > There is still something that would simplify what is done here: it's the
> > "the elimination of the write & sync columns for pg_stat_wal" mentioned in [2].
> 
> Yeah, still you cannot just remove them because the data tracked in
> pg_stat_io is not entirely the same, right?

I think that we can just remove them. They are tracked and incremented at the
exact same places in issue_xlog_fsync() and XLogWrite(). What differs is the
"bytes" (as pg_stat_wal.wal_bytes somehow "focus" on the wal records size while
the pg_stat_io's unit is the wal_block_size) and we keep them in both places.
Also it looks like we can get rid of PendingWalStats...

Regards,
 
-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  30. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-02-17T15:14:59Z

    Hi,

On Mon, Feb 17, 2025 at 07:59:26AM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Mon, Feb 17, 2025 at 04:25:46PM +0900, Michael Paquier wrote:
> > On Mon, Feb 17, 2025 at 06:59:40AM +0000, Bertrand Drouvot wrote:
> > > There is still something that would simplify what is done here: it's the
> > > "the elimination of the write & sync columns for pg_stat_wal" mentioned in [2].
> > 
> > Yeah, still you cannot just remove them because the data tracked in
> > pg_stat_io is not entirely the same, right?
> 
> I think that we can just remove them. They are tracked and incremented at the
> exact same places in issue_xlog_fsync() and XLogWrite(). What differs is the
> "bytes" (as pg_stat_wal.wal_bytes somehow "focus" on the wal records size while
> the pg_stat_io's unit is the wal_block_size) and we keep them in both places.
> Also it looks like we can get rid of PendingWalStats...

PFA the whole picture. 0001 is implementing the fields removal in pg_stat_wal
(and also PendingWalStats). I think that's ok given the backend's type for which
pgstat_tracks_io_bktype() returns false. But now you make me doubt about 0001.
Anyway, it's probably better to move the 0001 discussion to a dedicated thread,
thoughts?

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  31. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-02-17T23:34:32Z

    On Mon, Feb 17, 2025 at 03:14:59PM +0000, Bertrand Drouvot wrote:
> PFA the whole picture. 0001 is implementing the fields removal in pg_stat_wal
> (and also PendingWalStats). I think that's ok given the backend's type for which
> pgstat_tracks_io_bktype() returns false. But now you make me doubt about 0001.

Double-checking the code now and my doubts are wrong.

I think that I would vote for a removal of the fields from pg_stat_wal
rather than a replacement in pg_stat_wal, for the following reasons:
- pg_stat_stat.wal_write is the same value as "select sum(writes)
from pg_stat_io where object = 'wal' and context = 'normal'" as these
are incremented in XLogWrite().
- Same argument about pg_stat_wal.wal_write_time with
pg_stat_io.write_time.
- issue_xlog_fsync() tells that pg_stat_wal.wal_sync_time and
sum(pg_stat_io.fsync_time) under object=wal and context=normal are the
same values.
- Same argument with the fsync counters pg_stat_wal.wal_sync and
pg_stat_io.fsyncs.
- Encourage monitoring pull to move to pg_stat_io, where there is much
more context and granularity of the stats data.

Regarding the GUC track_wal_io_timing, my take is that we'll live
better if we just let it go.  It loses its meaning once pg_stat_wal
does not track the write and sync timings.

> Anyway, it's probably better to move the 0001 discussion to a dedicated thread,
> thoughts?

Yes.  And we cannot really move forward with what we have here without
deciding about this part.  The simplifications I can read from
v7-0002~v7-0004 are really nice.  These make the implementation of WAL
stats at backend-level really simpler to think about.

The doc additions of v7-0001 about the description of what the 'wal'
object does in pg_stat_io are actually worth a change of their own?
We already track them in pg_stat_io.
--
Michael

  32. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-02-18T10:45:29Z

    Hi,

On Tue, Feb 18, 2025 at 08:34:32AM +0900, Michael Paquier wrote:
> On Mon, Feb 17, 2025 at 03:14:59PM +0000, Bertrand Drouvot wrote:
> > PFA the whole picture. 0001 is implementing the fields removal in pg_stat_wal
> > (and also PendingWalStats). I think that's ok given the backend's type for which
> > pgstat_tracks_io_bktype() returns false. But now you make me doubt about 0001.
> 
> Double-checking the code now and my doubts are wrong.

Thanks for double checking.

> I think that I would vote for a removal of the fields from pg_stat_wal
> rather than a replacement in pg_stat_wal, for the following reasons:
> - pg_stat_stat.wal_write is the same value as "select sum(writes)
> from pg_stat_io where object = 'wal' and context = 'normal'" as these
> are incremented in XLogWrite().
> - Same argument about pg_stat_wal.wal_write_time with
> pg_stat_io.write_time.
> - issue_xlog_fsync() tells that pg_stat_wal.wal_sync_time and
> sum(pg_stat_io.fsync_time) under object=wal and context=normal are the
> same values.
> - Same argument with the fsync counters pg_stat_wal.wal_sync and
> pg_stat_io.fsyncs.
> - Encourage monitoring pull to move to pg_stat_io, where there is much
> more context and granularity of the stats data.

Agree with all of the above + pgstat_tracks_io_bktype() returns false for backend's
that do not generate WAL (so that we don't lose WAL information in pg_stat_io).

> Regarding the GUC track_wal_io_timing, my take is that we'll live
> better if we just let it go.  It loses its meaning once pg_stat_wal
> does not track the write and sync timings.

Yeah, done that way in the dedicated thread ([1]).

> > Anyway, it's probably better to move the 0001 discussion to a dedicated thread,
> > thoughts?
> 
> Yes.  And we cannot really move forward with what we have here without
> deciding about this part.  The simplifications I can read from
> v7-0002~v7-0004 are really nice.  These make the implementation of WAL
> stats at backend-level really simpler to think about.

yup.

> The doc additions of v7-0001 about the description of what the 'wal'
> object does in pg_stat_io are actually worth a change of their own?
> We already track them in pg_stat_io.

Agree, done that way in the dedicated thread ([1]).

[1]: https://www.postgresql.org/message-id/flat/Z7RkQ0EfYaqqjgz/%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  33. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-02-18T22:28:49Z

    On Tue, Feb 18, 2025 at 10:45:29AM +0000, Bertrand Drouvot wrote:
> Agree, done that way in the dedicated thread ([1]).
> 
> [1]: https://www.postgresql.org/message-id/flat/Z7RkQ0EfYaqqjgz/%40ip-10-97-1-34.eu-west-3.compute.internal

Thanks for splitting this part into its own thread.
--
Michael

  34. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-02-24T09:07:39Z

    Hi,

On Wed, Feb 19, 2025 at 07:28:49AM +0900, Michael Paquier wrote:
> On Tue, Feb 18, 2025 at 10:45:29AM +0000, Bertrand Drouvot wrote:
> > Agree, done that way in the dedicated thread ([1]).
> > 
> > [1]: https://www.postgresql.org/message-id/flat/Z7RkQ0EfYaqqjgz/%40ip-10-97-1-34.eu-west-3.compute.internal
> 
> Thanks for splitting this part into its own thread.

Now that 2421e9a51d2 is in, let's resume working in this thread. PFA a rebase to
make the CF bot happy. Nothing has changed since V7, V8 only removes "v7-0001" (
as part of 2421e9a51d2), so that v8-000N is nothing but v7-000(N+1).

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  35. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-02-25T06:50:38Z

    On Mon, Feb 24, 2025 at 09:07:39AM +0000, Bertrand Drouvot wrote:
> Now that 2421e9a51d2 is in, let's resume working in this thread. PFA a rebase to
> make the CF bot happy. Nothing has changed since V7, V8 only removes "v7-0001" (
> as part of 2421e9a51d2), so that v8-000N is nothing but v7-000(N+1).

v7-0001 looks sensible, so does v7-0002 with the introduction of
PgStat_WalCounters to tackle the fact that backend statistics need
only one reset_timestamp shared across IO and WAL stats.

+/*
+ * To determine whether WAL usage happened.
+ */
+static bool
+pgstat_backend_wal_have_pending(void)
+{
+	return pgWalUsage.wal_records != prevBackendWalUsage.wal_records;
+}

Okay for this pending data check.

--- a/src/backend/utils/activity/pgstat_wal.c
+++ b/src/backend/utils/activity/pgstat_wal.c
@@ -53,6 +53,8 @@ pgstat_report_wal(bool force)
 	/* flush wal stats */
 	pgstat_flush_wal(nowait);
 
+	pgstat_flush_backend(nowait, PGSTAT_BACKEND_FLUSH_WAL);
+
 	/* flush IO stats */
 	pgstat_flush_io(nowait);

Fine to stick that into pgstat_report_wal(), which is used anywhere
else.  pgstat_flush_wal() could be static in pgstat_wal.c? 

+Datum
+pg_stat_get_backend_wal(PG_FUNCTION_ARGS)
+{
[...]
+    pid = PG_GETARG_INT32(0);
+    proc = BackendPidGetProc(pid);
+
+    /*
+     * This could be an auxiliary process but these do not report backend
+     * statistics due to pgstat_tracks_backend_bktype(), so there is no need
+     * for an extra call to AuxiliaryPidGetProc().
+     */
+    if (!proc)
+        PG_RETURN_NULL();
+
+    procNumber = GetNumberFromPGProc(proc);
+
+    beentry = pgstat_get_beentry_by_proc_number(procNumber);
+    if (!beentry)
+        PG_RETURN_NULL();
+
+    backend_stats = pgstat_fetch_stat_backend(procNumber);
+    if (!backend_stats)
+        PG_RETURN_NULL();
+
+    /* if PID does not match, leave */
+    if (beentry->st_procpid != pid)
+        PG_RETURN_NULL();
+
+    /* backend may be gone, so recheck in case */
+    if (beentry->st_backendType == B_INVALID)
+        PG_RETURN_NULL();

This is a block of code copy-pasted from pg_stat_get_backend_io().
This is complex, so perhaps it would be better to refactor that in a
single routine that returns PgStat_Backend?  Then reuse the refactored
code in both pg_stat_get_backend_io() and the new
pg_stat_get_backend_wal().

+-- Test pg_stat_get_backend_wal (and make a temp table so our temp schema exists)
+SELECT wal_bytes AS backend_wal_bytes_before from pg_stat_get_backend_wal(pg_backend_pid()) \gset
+
 CREATE TEMP TABLE test_stats_temp AS SELECT 17;
 DROP TABLE test_stats_temp;
[...]
+SELECT pg_stat_force_next_flush();
+SELECT wal_bytes > :backend_wal_bytes_before FROM pg_stat_get_backend_wal(pg_backend_pid());

That should be stable, as we're guaranteed to have records here.
--
Michael

  36. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-02-25T15:00:35Z

    Hi,

On Tue, Feb 25, 2025 at 03:50:38PM +0900, Michael Paquier wrote:
> On Mon, Feb 24, 2025 at 09:07:39AM +0000, Bertrand Drouvot wrote:
> > Now that 2421e9a51d2 is in, let's resume working in this thread. PFA a rebase to
> > make the CF bot happy. Nothing has changed since V7, V8 only removes "v7-0001" (
> > as part of 2421e9a51d2), so that v8-000N is nothing but v7-000(N+1).
> 
> v7-0001 looks sensible, so does v7-0002 with the introduction of
> PgStat_WalCounters to tackle the fact that backend statistics need
> only one reset_timestamp shared across IO and WAL stats.

Thanks for looking at it! (I guess you meant to say v8-0001 and v8-0002).

> --- a/src/backend/utils/activity/pgstat_wal.c
> +++ b/src/backend/utils/activity/pgstat_wal.c
> @@ -53,6 +53,8 @@ pgstat_report_wal(bool force)
>  	/* flush wal stats */
>  	pgstat_flush_wal(nowait);
>  
> +	pgstat_flush_backend(nowait, PGSTAT_BACKEND_FLUSH_WAL);
> +
>  	/* flush IO stats */
>  	pgstat_flush_io(nowait);
> 
> Fine to stick that into pgstat_report_wal(), which is used anywhere
> else.  pgstat_flush_wal() could be static in pgstat_wal.c? 

hmm right. Not linked to this patch though, so done in a dedicated patch
in passing (v9-0001).

> +Datum
> +pg_stat_get_backend_wal(PG_FUNCTION_ARGS)
> +{
> [...]
> +    pid = PG_GETARG_INT32(0);
> +    proc = BackendPidGetProc(pid);
> +
> +    /*
> +     * This could be an auxiliary process but these do not report backend
> +     * statistics due to pgstat_tracks_backend_bktype(), so there is no need
> +     * for an extra call to AuxiliaryPidGetProc().
> +     */
> +    if (!proc)
> +        PG_RETURN_NULL();
> +
> +    procNumber = GetNumberFromPGProc(proc);
> +
> +    beentry = pgstat_get_beentry_by_proc_number(procNumber);
> +    if (!beentry)
> +        PG_RETURN_NULL();
> +
> +    backend_stats = pgstat_fetch_stat_backend(procNumber);
> +    if (!backend_stats)
> +        PG_RETURN_NULL();
> +
> +    /* if PID does not match, leave */
> +    if (beentry->st_procpid != pid)
> +        PG_RETURN_NULL();
> +
> +    /* backend may be gone, so recheck in case */
> +    if (beentry->st_backendType == B_INVALID)
> +        PG_RETURN_NULL();
> 
> This is a block of code copy-pasted from pg_stat_get_backend_io().
> This is complex, so perhaps it would be better to refactor that in a
> single routine that returns PgStat_Backend?  Then reuse the refactored
> code in both pg_stat_get_backend_io() and the new
> pg_stat_get_backend_wal().

That makes fully sense. Done in 0004 attached. Somehow related to that, I've
a patch in progress to address some of Rahila's comments ([1]) (the one related
to the AuxiliaryPidGetProc() call is relevant specially since a051e71e28a where
pgstat_tracks_backend_bktype() has been modified for B_WAL_RECEIVER, B_WAL_SUMMARIZER
and B_WAL_WRITER). I'll wait for 0004 to go in before sharing the patch.

> +-- Test pg_stat_get_backend_wal (and make a temp table so our temp schema exists)
> +SELECT wal_bytes AS backend_wal_bytes_before from pg_stat_get_backend_wal(pg_backend_pid()) \gset
> +
>  CREATE TEMP TABLE test_stats_temp AS SELECT 17;
>  DROP TABLE test_stats_temp;
> [...]
> +SELECT pg_stat_force_next_flush();
> +SELECT wal_bytes > :backend_wal_bytes_before FROM pg_stat_get_backend_wal(pg_backend_pid());
> 
> That should be stable, as we're guaranteed to have records here.

Yup.

[1]: https://www.postgresql.org/message-id/CAH2L28v9BwN8_y0k6FQ591%3D0g2Hj_esHLGj3bP38c9nmVykoiA%40mail.gmail.com

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  37. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-02-26T07:52:13Z

    On Tue, Feb 25, 2025 at 03:00:35PM +0000, Bertrand Drouvot wrote:
> That makes fully sense. Done in 0004 attached. Somehow related to that, I've
> a patch in progress to address some of Rahila's comments ([1]) (the one related
> to the AuxiliaryPidGetProc() call is relevant specially since a051e71e28a where
> pgstat_tracks_backend_bktype() has been modified for B_WAL_RECEIVER, B_WAL_SUMMARIZER
> and B_WAL_WRITER). I'll wait for 0004 to go in before sharing the patch.

Applied v9-0001 and v9-0003 as these were fine, with more
documentation added in pgstat.h for the new WAL structure, and the 
reason why it exists.  I've noticed the difference with bktype in
v9-0004 as the WAL part does not need this information when generating
its tuple, OK here.

Doing v9-0003 after v9-0002 felt a bit odd, changing twice the
signature of pg_stat_wal_build_tuple() to adapt with the split for the
reset timestamp.

-	values[4] = TimestampTzGetDatum(wal_stats->stat_reset_timestamp);
+	if (wal_stats.stat_reset_timestamp != 0)
+		values[4] = TimestampTzGetDatum(wal_stats.stat_reset_timestamp);
+	else
+		nulls[4] = true;

In patch v9-0002, is this nulls[4] required for the backend part?
--
Michael

  38. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-02-26T10:59:11Z

    Hi,

On Wed, Feb 26, 2025 at 04:52:13PM +0900, Michael Paquier wrote:
> On Tue, Feb 25, 2025 at 03:00:35PM +0000, Bertrand Drouvot wrote:
> > That makes fully sense. Done in 0004 attached. Somehow related to that, I've
> > a patch in progress to address some of Rahila's comments ([1]) (the one related
> > to the AuxiliaryPidGetProc() call is relevant specially since a051e71e28a where
> > pgstat_tracks_backend_bktype() has been modified for B_WAL_RECEIVER, B_WAL_SUMMARIZER
> > and B_WAL_WRITER). I'll wait for 0004 to go in before sharing the patch.
> 
> Applied v9-0001

I see that you removed pgstat_flush_wal() in d7cbeaf261d (instead of what 0001
was doing i.e making it static). Makes sense to me.a

> and v9-0003 as these were fine,

Thanks.

> with more
> documentation added in pgstat.h for the new WAL structure, and the 
> reason why it exists.

Saw that, looks good.

> I've noticed the difference with bktype in
> v9-0004 as the WAL part does not need this information when generating
> its tuple, OK here.

Thx.

> Doing v9-0003 after v9-0002 felt a bit odd, changing twice the
> signature of pg_stat_wal_build_tuple() to adapt with the split for the
> reset timestamp.

PFA a rebase.

> -	values[4] = TimestampTzGetDatum(wal_stats->stat_reset_timestamp);
> +	if (wal_stats.stat_reset_timestamp != 0)
> +		values[4] = TimestampTzGetDatum(wal_stats.stat_reset_timestamp);
> +	else
> +		nulls[4] = true;
> 
> In patch v9-0002, is this nulls[4] required for the backend part?

Yup. That's what we've done in pg_stat_io_build_tuples() too (ff7c40d7fd6).
Without this we'd get "2000-01-01 00:00:00+00" in the stats_reset field of
pg_stat_get_backend_wal() and pg_stat_get_backend_io().

That was not needed for pg_stat_io and pg_stat_wal because the stats_reset field
was already non null after initdb.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  39. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-02-27T03:02:51Z

    On Wed, Feb 26, 2025 at 10:59:11AM +0000, Bertrand Drouvot wrote:
> Yup. That's what we've done in pg_stat_io_build_tuples() too (ff7c40d7fd6).
> Without this we'd get "2000-01-01 00:00:00+00" in the stats_reset field of
> pg_stat_get_backend_wal() and pg_stat_get_backend_io().

Right, forgot about this part.

> That was not needed for pg_stat_io and pg_stat_wal because the stats_reset field
> was already non null after initdb.

0001 was OK, so done.

In 0002, couldn't it be better to have the pg_stat_get_backend_stats()
static in pgstatfuncs.c?  In 0003, pg_stat_get_backend_wal() is also
in pgstatfuncs.c, meaning that all the callers of
pg_stat_get_backend_stats() would be in this file.

-typedef struct PgStat_Backend
-{
-	TimestampTz stat_reset_timestamp;
-	PgStat_BktypeIO io_stats;
-} PgStat_Backend;
-
 /* ---------
  * PgStat_BackendPending	Non-flushed backend stats.
  * ---------

In 0003, let's keep PgStat_BackendPending grouped with PgStat_Backend,
so it sounds better to move both of them after the WAL stats
structures.
--
Michael

  40. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-02-27T07:47:09Z

    Hi,

On Thu, Feb 27, 2025 at 12:02:51PM +0900, Michael Paquier wrote:
> 0001 was OK, so done.

Thanks!

> In 0002, couldn't it be better to have the pg_stat_get_backend_stats()
> static in pgstatfuncs.c?  In 0003, pg_stat_get_backend_wal() is also
> in pgstatfuncs.c, meaning that all the callers of
> pg_stat_get_backend_stats() would be in this file.

That's how I did it initially but decided to move it to pgstat_backend.c. The
reason was that it's fully linked to "per backend" stats and that there is
no SQL api on top of it (while I think that's the case for almost all the ones
in pgstatfuncs.c). Thoughts?

> -typedef struct PgStat_Backend
> -{
> -	TimestampTz stat_reset_timestamp;
> -	PgStat_BktypeIO io_stats;
> -} PgStat_Backend;
> -
>  /* ---------
>   * PgStat_BackendPending	Non-flushed backend stats.
>   * ---------
> 
> In 0003, let's keep PgStat_BackendPending grouped with PgStat_Backend,
> so it sounds better to move both of them after the WAL stats
> structures.

Makes sense. I did not had in mind to submit a new patch version (to at least
implement the above) without getting your final thoughts on your first comment.
But since a rebase is needed anyway,then please find attached a new version. It
just implements your last comment.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  41. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-02-28T02:34:14Z

    On Thu, Feb 27, 2025 at 07:47:09AM +0000, Bertrand Drouvot wrote:
> That's how I did it initially but decided to move it to pgstat_backend.c. The
> reason was that it's fully linked to "per backend" stats and that there is
> no SQL api on top of it (while I think that's the case for almost all the ones
> in pgstatfuncs.c). Thoughts?

Okay by me with pgstat_fetch_stat_backend in parallel, why not
exposing this part as well..  Perhaps that could be useful for some
extension?  I'd rather have out-of-core code do these lookups with the
same sanity checks in place for the procnumber and slot lookups.

The name was inconsistent with the rest of the file, so I have settled
to a pgstat_fetch_stat_backend_by_pid() to be more consistent.  A
second thing is to properly initialize bktype if defined by the
caller.

> Makes sense. I did not had in mind to submit a new patch version (to at least
> implement the above) without getting your final thoughts on your first comment.
> But since a rebase is needed anyway,then please find attached a new version. It
> just implements your last comment.

Attached is a rebased version of the rest.
--
Michael

  42. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-02-28T09:26:08Z

    Hi,

On Fri, Feb 28, 2025 at 11:34:14AM +0900, Michael Paquier wrote:
> On Thu, Feb 27, 2025 at 07:47:09AM +0000, Bertrand Drouvot wrote:
> > That's how I did it initially but decided to move it to pgstat_backend.c. The
> > reason was that it's fully linked to "per backend" stats and that there is
> > no SQL api on top of it (while I think that's the case for almost all the ones
> > in pgstatfuncs.c). Thoughts?
> 
> Okay by me with pgstat_fetch_stat_backend in parallel, why not
> exposing this part as well..  Perhaps that could be useful for some
> extension?  I'd rather have out-of-core code do these lookups with the
> same sanity checks in place for the procnumber and slot lookups.

Yeah that's also a pros for it.

> The name was inconsistent with the rest of the file, so I have settled
> to a pgstat_fetch_stat_backend_by_pid() to be more consistent.

Sounds good, thanks!

> A
> second thing is to properly initialize bktype if defined by the
> caller.

Saw that in c2a50ac678e, makes sense.

> Attached is a rebased version of the rest.

The rebased version looks ok.

Also attaching the patch I mentioned up-thread to address some of Rahila's
comments ([1]): It adds a AuxiliaryPidGetProc() call in pgstat_fetch_stat_backend_by_pid()
and pg_stat_reset_backend_stats(). I think that fully makes sense since a051e71e28a
modified pgstat_tracks_backend_bktype() for B_WAL_RECEIVER, B_WAL_SUMMARIZER
and B_WAL_WRITER.

It looks like it does not need doc updates. Attached as 0002 as it's somehow
un-related to this thread (but not sure it deserves it's dedicated thread though).

[1]: https://www.postgresql.org/message-id/CAH2L28v9BwN8_y0k6FQ591=0g2Hj_esHLGj3bP38c9nmVykoiA@mail.gmail.com
[2]: https://www.postgresql.org/message-id/flat/Z8FMjlyNpNicucGa%40paquier.xyz#92d3e2b9ad860708f76b8f9c6f49f32d

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  43. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-02-28T12:22:16Z

    On Fri, Feb 28, 2025 at 09:26:08AM +0000, Bertrand Drouvot wrote:
> Also attaching the patch I mentioned up-thread to address some of Rahila's
> comments ([1]): It adds a AuxiliaryPidGetProc() call in pgstat_fetch_stat_backend_by_pid()
> and pg_stat_reset_backend_stats(). I think that fully makes sense since a051e71e28a
> modified pgstat_tracks_backend_bktype() for B_WAL_RECEIVER, B_WAL_SUMMARIZER
> and B_WAL_WRITER.

Oops, yes, you are right on this one.  This change should have
happened earlier.  The flow you are using in 0002 is similar to
pg_log_backend_memory_contexts(), which looks OK at quick glance.
--
Michael

  44. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-03-03T01:48:23Z

    On Fri, Feb 28, 2025 at 09:26:08AM +0000, Bertrand Drouvot wrote:
> Also attaching the patch I mentioned up-thread to address some of Rahila's
> comments ([1]): It adds a AuxiliaryPidGetProc() call in pgstat_fetch_stat_backend_by_pid()
> and pg_stat_reset_backend_stats(). I think that fully makes sense since a051e71e28a
> modified pgstat_tracks_backend_bktype() for B_WAL_RECEIVER, B_WAL_SUMMARIZER
> and B_WAL_WRITER.

Okay by me as it makes the code automatically more flexible if
pgstat_tracks_backend_bktype() gets tweaked, including the call of
pgstat_flush_backend() in pgstat_report_wal() so as the WAL writer is
able to report backend stats for its WAL I/O.  Applied this part as of
3f1db99bfabb.

Something that's still not quite right is that the WAL receiver and
the WAL summarizer do not call pgstat_report_wal() at all, so we don't
report much data and we expect these processes to run continuously.
The location where to report stats for the WAL summarizer is simple,
even if the system is aggressive with WAL this is never called more
than a couple of times per seconds, like the WAL writer:

@@ -1541,6 +1542,10 @@ summarizer_read_local_xlog_page(XLogReaderState *state,
                  * so we don't tight-loop.
                  */
                 HandleWalSummarizerInterrupts();
+
+                /* report pending statistics to the cumulative stats system */
+                pgstat_report_wal(false);
+
                 summarizer_wait_for_wal();

At this location, the WAL summarizer would wait as there is no data to
read.  The hot path is when we're reading a block.

The WAL receiver is a different story, because the WaitLatchOrSocket()
call in the main loop of WalReceiverMain() is *very* aggressive, and
it's easy to reach this code dozens of times each millisecond.  In
short, we need to be careful, I think, based on how this is currently
written.  My choice is then this path:
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -583,6 +583,10 @@ WalReceiverMain(const void *startup_data, size_t startup_data_len)
                      */
                     bool        requestReply = false;
 
+                    /* report pending statistics to the cumulative stats system */
+                    pgstat_report_wal(false);
+
                     /*
                      * Check if time since last receive from prim

This would update the stats only when the WAL receiver has nothing to
do or if wal_receiver_status_interval is reached, so we're not going
to storm pgstats with updates, still we get some data on a periodic
basis *because* wal_receiver_status_interval would make sure that the
path is taken even if we're under a lot of WAL pressure when sending
feedback messages back to the WAL sender.  Of course this needs a
pretty good comment explaining the choice of this location.  What do
you think?

> It looks like it does not need doc updates. Attached as 0002 as it's somehow
> un-related to this thread (but not sure it deserves it's dedicated thread though).

I'm wondering if we should not lift more the list of processes listed
in pgstat_tracks_backend_bktype() and include B_AUTOVAC_LAUNCHER,
B_STARTUP, B_CHECKPOINTER, B_BG_WRITER at this stage, removing the
entire paragraph.  Not sure if we really have to do that for this
release, but we could look at that separately.

With 3f1db99bfabb in place, wouldn't it be simpler to update
pgstat_report_wal() in v12-0001 so as we use PGSTAT_BACKEND_FLUSH_ALL
with one call of pgstat_flush_backend()?  This saves one call for each
stats flush.
--
Michael

  45. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-03-03T09:17:30Z

    Hi,

On Mon, Mar 03, 2025 at 10:48:23AM +0900, Michael Paquier wrote:
> On Fri, Feb 28, 2025 at 09:26:08AM +0000, Bertrand Drouvot wrote:
> > Also attaching the patch I mentioned up-thread to address some of Rahila's
> > comments ([1]): It adds a AuxiliaryPidGetProc() call in pgstat_fetch_stat_backend_by_pid()
> > and pg_stat_reset_backend_stats(). I think that fully makes sense since a051e71e28a
> > modified pgstat_tracks_backend_bktype() for B_WAL_RECEIVER, B_WAL_SUMMARIZER
> > and B_WAL_WRITER.
> 
> Okay by me as it makes the code automatically more flexible if
> pgstat_tracks_backend_bktype() gets tweaked, including the call of
> pgstat_flush_backend() in pgstat_report_wal() so as the WAL writer is
> able to report backend stats for its WAL I/O.  Applied this part as of
> 3f1db99bfabb.

Thanks!

> Something that's still not quite right is that the WAL receiver and
> the WAL summarizer do not call pgstat_report_wal() at all, so we don't
> report much data and we expect these processes to run continuously.
> The location where to report stats for the WAL summarizer is simple,
> even if the system is aggressive with WAL this is never called more
> than a couple of times per seconds, like the WAL writer:
> 
> @@ -1541,6 +1542,10 @@ summarizer_read_local_xlog_page(XLogReaderState *state,
>                   * so we don't tight-loop.
>                   */
>                  HandleWalSummarizerInterrupts();
> +
> +                /* report pending statistics to the cumulative stats system */
> +                pgstat_report_wal(false);
> +
>                  summarizer_wait_for_wal();
> 
> At this location, the WAL summarizer would wait as there is no data to
> read.  The hot path is when we're reading a block.

Did not look closely enough but that sounds right after a quick look.

> The WAL receiver is a different story, because the WaitLatchOrSocket()
> call in the main loop of WalReceiverMain() is *very* aggressive, and
> it's easy to reach this code dozens of times each millisecond.  In
> short, we need to be careful, I think, based on how this is currently
> written.  My choice is then this path:
> --- a/src/backend/replication/walreceiver.c
> +++ b/src/backend/replication/walreceiver.c
> @@ -583,6 +583,10 @@ WalReceiverMain(const void *startup_data, size_t startup_data_len)
>                       */
>                      bool        requestReply = false;
>  
> +                    /* report pending statistics to the cumulative stats system */
> +                    pgstat_report_wal(false);
> +
>                      /*
>                       * Check if time since last receive from prim
> 
> This would update the stats only when the WAL receiver has nothing to
> do or if wal_receiver_status_interval is reached, so we're not going
> to storm pgstats with updates, still we get some data on a periodic
> basis *because* wal_receiver_status_interval would make sure that the
> path is taken even if we're under a lot of WAL pressure when sending
> feedback messages back to the WAL sender.  Of course this needs a
> pretty good comment explaining the choice of this location.  What do
> you think?

Same as above, that sounds right after a quick look.

> > It looks like it does not need doc updates. Attached as 0002 as it's somehow
> > un-related to this thread (but not sure it deserves it's dedicated thread though).
> 
> I'm wondering if we should not lift more the list of processes listed
> in pgstat_tracks_backend_bktype() and include B_AUTOVAC_LAUNCHER,
> B_STARTUP, B_CHECKPOINTER, B_BG_WRITER at this stage, removing the
> entire paragraph.  Not sure if we really have to do that for this
> release, but we could look at that separately.

hm, do you mean update the comment on top of pgstat_tracks_backend_bktype() or
update the documentation?

> With 3f1db99bfabb in place, wouldn't it be simpler to update
> pgstat_report_wal() in v12-0001 so as we use PGSTAT_BACKEND_FLUSH_ALL
> with one call of pgstat_flush_backend()?  This saves one call for each
> stats flush.

hmm, that would work as long as PGSTAT_BACKEND_FLUSH_ALL represents things
that need to be called from pgstat_report_wal(). But I think that's open
door for issue should be add a new PGSTAT_BACKEND_FLUSH_XXX where XXX is not
related to pgstat_report_wal() at all. So, I'm tempted to keep it as it is.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  46. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-03-03T10:47:28Z

    Hi,

On Mon, Mar 03, 2025 at 09:17:30AM +0000, Bertrand Drouvot wrote:
> hmm, that would work as long as PGSTAT_BACKEND_FLUSH_ALL represents things
> that need to be called from pgstat_report_wal(). But I think that's open
> door for issue should be add a new PGSTAT_BACKEND_FLUSH_XXX where XXX is not
> related to pgstat_report_wal() at all. So, I'm tempted to keep it as it is.

I just realized that pgstat_flush_backend() is not correct in 0001. Indeed
we check:

"
    if (pg_memory_is_all_zeros(&PendingBackendStats,
                               sizeof(struct PgStat_BackendPending)))
        return false;
"

but the WAL stats are not part of PgStat_BackendPending... So we only check
for IO pending stats here. I'm not sure WAL stats could be non empty if IO 
stats are but the attached now also takes care of pending WAL stats here.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  47. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-03-04T00:28:27Z

    On Mon, Mar 03, 2025 at 09:17:30AM +0000, Bertrand Drouvot wrote:
> On Mon, Mar 03, 2025 at 10:48:23AM +0900, Michael Paquier wrote:
>> Something that's still not quite right is that the WAL receiver and
>> the WAL summarizer do not call pgstat_report_wal() at all, so we don't
>> report much data and we expect these processes to run continuously.
>> The location where to report stats for the WAL summarizer is simple,
>> even if the system is aggressive with WAL this is never called more
>> than a couple of times per seconds, like the WAL writer:
> 
> Same as above, that sounds right after a quick look.

Attached is a patch for this set of issues for the WAL receiver, the
WAL summarizer and the WAL writer.  Another thing that we can do
better is restrict pgstat_tracks_io_object() so as we don't report
rows for non-WAL IOObject in the case of these three.  Two tests are
added for the WAL receiver and WAL summarizer, checking that the stats
are gathered for both.  For the WAL receiver, we have at least the
activity coming from one WAL segment created in the init context, at
least.  The WAL summarizer is more pro-active with its reads in its
TAP test.

All that should be fixed before looking at the remaining patch for the
WAL stats at backend level, so what do you think about the attached?

>> I'm wondering if we should not lift more the list of processes listed
>> in pgstat_tracks_backend_bktype() and include B_AUTOVAC_LAUNCHER,
>> B_STARTUP, B_CHECKPOINTER, B_BG_WRITER at this stage, removing the
>> entire paragraph.  Not sure if we really have to do that for this
>> release, but we could look at that separately.
> 
> hm, do you mean update the comment on top of pgstat_tracks_backend_bktype() or
> update the documentation?

My argument would be to make pgstat_tracks_backend_bktype() the same
as pgstat_io.c, and reflect that in the docs and the comments.

> hmm, that would work as long as PGSTAT_BACKEND_FLUSH_ALL represents things
> that need to be called from pgstat_report_wal(). But I think that's open
> door for issue should be add a new PGSTAT_BACKEND_FLUSH_XXX where XXX is not
> related to pgstat_report_wal() at all. So, I'm tempted to keep it as it is.

OK, I can see your point here.  Fine by me.
--
Michael

  48. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-03-04T08:48:28Z

    Hi,

On Tue, Mar 04, 2025 at 09:28:27AM +0900, Michael Paquier wrote:
> On Mon, Mar 03, 2025 at 09:17:30AM +0000, Bertrand Drouvot wrote:
> > On Mon, Mar 03, 2025 at 10:48:23AM +0900, Michael Paquier wrote:
> >> Something that's still not quite right is that the WAL receiver and
> >> the WAL summarizer do not call pgstat_report_wal() at all, so we don't
> >> report much data and we expect these processes to run continuously.
> >> The location where to report stats for the WAL summarizer is simple,
> >> even if the system is aggressive with WAL this is never called more
> >> than a couple of times per seconds, like the WAL writer:
> > 
> > Same as above, that sounds right after a quick look.
> 
> Attached is a patch for this set of issues for the WAL receiver, the
> WAL summarizer and the WAL writer.

Thanks for the patch!

=== 1

@@ -1543,6 +1544,9 @@ summarizer_read_local_xlog_page(XLogReaderState *state,
                                HandleWalSummarizerInterrupts();
                                summarizer_wait_for_wal();

+                               /* report pending statistics to the cumulative stats system */
+                               pgstat_report_wal(false);

s/report/Report/ and s/system/system./? to be consistent with the other single
line comments around.

=== 2

+  /*
+   * Report pending statistics to the cumulative stats
+   * system. This location is useful for the report as it is
+   * not within a tight loop in the WAL receiver, which
+   * would bloat requests to pgstats, while also making sure
+   * that the reports happen at least each time a status
+   * update is sent.
+   */

Yeah, I also think that's the right location.

Nit: s/would/could/?

=== 3

+       /*
+        * Some BackendTypes only perform IO under IOOBJECT_WAL, hence exclude all
+        * rows for all the other objects for these.
+        */
+       if ((bktype == B_WAL_SUMMARIZER || bktype == B_WAL_RECEIVER ||
+                bktype == B_WAL_WRITER) && io_object != IOOBJECT_WAL)
+               return false;

I think that makes sense and it removes 15 lines out of 86. This function is
"hard" to read/parse from my point of view. Maybe we could re-write it in a
simpler way but that's outside the purpose of this thread. 

=== 4

+   WHERE backend_type = 'walsummarizer' AND object = 'wal'});

The object = 'wal' is not needed (thanks to === 3), maybe we can remove this
filtering?

Also what about adding a test to check that sum(reads) is NULL where object != 'wal'?

=== 5

Same remark as above for the WAL receiver (excepts that sum(writes) is NULL where
object != 'wal').

> All that should be fixed before looking at the remaining patch for the
> WAL stats at backend level

Not sure as that would also prevent the other backend types to report their WAL
statistics if the above is not fixed. 

>, so what do you think about the attached?

That's pretty straightforward, so yeah we can wait that it goes in before
moving forward with the WAL stats at backend level.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  49. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-03-05T01:20:51Z

    On Tue, Mar 04, 2025 at 08:48:28AM +0000, Bertrand Drouvot wrote:
> s/report/Report/ and s/system/system./? to be consistent with the other single
> line comments around.

Right.

> Yeah, I also think that's the right location.

We could be more optimal for the WAL receiver if we add more timestamp
calculations, I think, but that's a sensitive loop, and this is better
than no information anyway.  If somebody has a better idea, feel free.
We could have an extra GUC to control that, but I'm feeling that we
should restructure the WAL receiver before that, perhaps leverage some
of its activity elsewhere (?).

> +       /*
> +        * Some BackendTypes only perform IO under IOOBJECT_WAL, hence exclude all
> +        * rows for all the other objects for these.
> +        */
> +       if ((bktype == B_WAL_SUMMARIZER || bktype == B_WAL_RECEIVER ||
> +                bktype == B_WAL_WRITER) && io_object != IOOBJECT_WAL)
> +               return false;
> 
> I think that makes sense and it removes 15 lines out of 86. This function is
> "hard" to read/parse from my point of view. Maybe we could re-write it in a
> simpler way but that's outside the purpose of this thread. 

One thing I am planning to do here to improve the situation is the
addition of a regression test that queries pg_stat_io for all the
combinations of backend_type, object and contexts that are now
allowed, to keep track of the number of tuples we have.

> +   WHERE backend_type = 'walsummarizer' AND object = 'wal'});
> 
> The object = 'wal' is not needed (thanks to === 3), maybe we can remove this
> filtering?
> 
> Also what about adding a test to check that sum(reads) is NULL where object != 'wal'?

Not sure it matters as long as we track the supported combinations.
We need something a bit more general here.

(I've actually found a different issue while looking at the WAL
receiver, which is a bit older than what we have here.  Will post that
in a different thread with you in CC.)
--
Michael

  50. Re: per backend WAL statistics

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-03-05T09:45:57Z

    Subject: Clarification Needed on WAL Pending Checks in Patchset

Hi,

Thank you for the patchset. I’ve spent some time learning and reviewing it
and have 2 comments.  I'm new to PostgreSQL hacking, so please bear with me
if I make mistakes or say something that seems trivial.

I noticed that in patches prior to patch 6, the function
pgstat_backend_wal_have_pending() was implemented as follows:

/*
 * To determine whether any WAL activity has occurred since last time, not
 * only the number of generated WAL records but also the numbers of WAL
 * writes and syncs need to be checked. Because even transactions that
 * generate no WAL records can write or sync WAL data when flushing the
 * data pages.
 */
static bool
pgstat_backend_wal_have_pending(void)
{
    PgStat_PendingWalStats pending_wal;

    pending_wal = PendingBackendStats.pending_wal;

    return pgWalUsage.wal_records != prevBackendWalUsage.wal_records ||
           pending_wal.wal_write != 0 || pending_wal.wal_sync != 0;
}

Starting with patch 7, it seems the implementation was simplified to:

/*
 * To determine whether WAL usage happened.
 */
static bool
pgstat_backend_wal_have_pending(void)
{
    return pgWalUsage.wal_records != prevBackendWalUsage.wal_records;
}

Meanwhile, the cluster-level check in the function
pgstat_wal_have_pending_cb() still performs the additional checks:

bool
pgstat_wal_have_pending_cb(void)
{
    return pgWalUsage.wal_records != prevWalUsage.wal_records ||
           PendingWalStats.wal_write != 0 ||
           PendingWalStats.wal_sync != 0;
}

The difference lies in the removal of the checks for wal_write and wal_sync
from the per-backend function. I assume that this may be due to factoring
out these counters—perhaps because they can now be extracted from
pg_stat_get_backend_io(). However, I’m not entirely sure I grasp the full
rationale behind this change.


Another one is on:
Bertrand Drouvot <bertranddrouvot.pg@gmail.com> 于2025年3月3日周一 18:47写道：

> Hi,
>
> On Mon, Mar 03, 2025 at 09:17:30AM +0000, Bertrand Drouvot wrote:
> > hmm, that would work as long as PGSTAT_BACKEND_FLUSH_ALL represents
> things
> > that need to be called from pgstat_report_wal(). But I think that's open
> > door for issue should be add a new PGSTAT_BACKEND_FLUSH_XXX where XXX is
> not
> > related to pgstat_report_wal() at all. So, I'm tempted to keep it as it
> is.
>
> I just realized that pgstat_flush_backend() is not correct in 0001. Indeed
> we check:
>
> "
>     if (pg_memory_is_all_zeros(&PendingBackendStats,
>                                sizeof(struct PgStat_BackendPending)))
>         return false;
> "
>
> but the WAL stats are not part of PgStat_BackendPending... So we only check
> for IO pending stats here. I'm not sure WAL stats could be non empty if IO
> stats are but the attached now also takes care of pending WAL stats here.
>
> Regards,
>
> --
> Bertrand Drouvot
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com


I've noticed that here's a function for checking if there are any backend
stats waiting for flush:
/*
 * Check if there are any backend stats waiting for flush.
 */
bool
pgstat_backend_have_pending_cb(void)
{
return (!pg_memory_is_all_zeros(&PendingBackendStats,
sizeof(struct PgStat_BackendPending)));
}

[PGSTAT_KIND_BACKEND] = {
 ....
.have_static_pending_cb = pgstat_backend_have_pending_cb,
.flush_static_cb = pgstat_backend_flush_cb,
.reset_timestamp_cb = pgstat_backend_reset_timestamp_cb,
},

Should the following modification be applied to the above function as well.
Or should we modify the comment 'any backend stat' if we choose to check
i/o only.
@@ -199,7 +258,8 @@ pgstat_flush_backend(bool nowait, bits32 flags)
  return false;

  if (pg_memory_is_all_zeros(&PendingBackendStats,
-   sizeof(struct PgStat_BackendPending)))
+   sizeof(struct PgStat_BackendPending))
+ && !pgstat_backend_wal_have_pending())
  return false;

Best regards,

[Xuneng]

  51. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-03-05T13:03:07Z

    Hi,

On Wed, Mar 05, 2025 at 05:45:57PM +0800, Xuneng Zhou wrote:
> Subject: Clarification Needed on WAL Pending Checks in Patchset
> 
> Hi,
> 
> Thank you for the patchset. I’ve spent some time learning and reviewing it
> and have 2 comments.

Thanks for looking at it!

> I noticed that in patches prior to patch 6, the function
> pgstat_backend_wal_have_pending() was implemented as follows:
> 
> /*
>  * To determine whether any WAL activity has occurred since last time, not
>  * only the number of generated WAL records but also the numbers of WAL
>  * writes and syncs need to be checked. Because even transactions that
>  * generate no WAL records can write or sync WAL data when flushing the
>  * data pages.
>  */
> static bool
> pgstat_backend_wal_have_pending(void)
> {
>     PgStat_PendingWalStats pending_wal;
> 
>     pending_wal = PendingBackendStats.pending_wal;
> 
>     return pgWalUsage.wal_records != prevBackendWalUsage.wal_records ||
>            pending_wal.wal_write != 0 || pending_wal.wal_sync != 0;
> }
> 
> Starting with patch 7, it seems the implementation was simplified to:
> 
> /*
>  * To determine whether WAL usage happened.
>  */
> static bool
> pgstat_backend_wal_have_pending(void)
> {
>     return pgWalUsage.wal_records != prevBackendWalUsage.wal_records;
> }

That's right. This is due to 2421e9a51d2 that removed PgStat_PendingWalStats.

> Meanwhile, the cluster-level check in the function
> pgstat_wal_have_pending_cb() still performs the additional checks:
> 
> bool
> pgstat_wal_have_pending_cb(void)
> {
>     return pgWalUsage.wal_records != prevWalUsage.wal_records ||
>            PendingWalStats.wal_write != 0 ||
>            PendingWalStats.wal_sync != 0;
> }

Not since 2421e9a51d2. It looks like that you are looking at code prior to
2421e9a51d2.

> Another one is on:
> Bertrand Drouvot <bertranddrouvot.pg@gmail.com> 于2025年3月3日周一 18:47写道：
> 
> > Hi,
> >
> > On Mon, Mar 03, 2025 at 09:17:30AM +0000, Bertrand Drouvot wrote:
> > > hmm, that would work as long as PGSTAT_BACKEND_FLUSH_ALL represents
> > things
> > > that need to be called from pgstat_report_wal(). But I think that's open
> > > door for issue should be add a new PGSTAT_BACKEND_FLUSH_XXX where XXX is
> > not
> > > related to pgstat_report_wal() at all. So, I'm tempted to keep it as it
> > is.
> >
> > I just realized that pgstat_flush_backend() is not correct in 0001. Indeed
> > we check:
> >
> > "
> >     if (pg_memory_is_all_zeros(&PendingBackendStats,
> >                                sizeof(struct PgStat_BackendPending)))
> >         return false;
> > "
> >
> > but the WAL stats are not part of PgStat_BackendPending... So we only check
> > for IO pending stats here. I'm not sure WAL stats could be non empty if IO
> > stats are but the attached now also takes care of pending WAL stats here.
> 
> I've noticed that here's a function for checking if there are any backend
> stats waiting for flush:
> /*
>  * Check if there are any backend stats waiting for flush.
>  */
> bool
> pgstat_backend_have_pending_cb(void)
> {
> return (!pg_memory_is_all_zeros(&PendingBackendStats,
> sizeof(struct PgStat_BackendPending)));
> }

That's right. 

The reason I did not add the extra check there is because I have in mind to
replace the pg_memory_is_all_zeros() checks by a new global variable and also replace
the checks in pgstat_flush_backend() by a call to pgstat_backend_have_pending_cb()
(see 0002 in [1]). It means that all of that would be perfectly clean if 
0002 in [1] goes in.

But yeah, if 0002 in [1] does not go in, then your concern is valid, so adding
the extra check in the attached.

Thanks for the review!

[1]: https://www.postgresql.org/message-id/Z8WYf1jyy4MwOveQ%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  52. Re: per backend WAL statistics

    Andres Freund <andres@anarazel.de> — 2025-03-05T14:18:16Z

    Hi,

On 2025-03-05 13:03:07 +0000, Bertrand Drouvot wrote:
> But yeah, if 0002 in [1] does not go in, then your concern is valid, so adding
> the extra check in the attached.

This crashes in cfbot:

https://cirrus-ci.com/task/5111872610893824

[13:42:37.315] src/tools/ci/cores_backtrace.sh freebsd /tmp/cores
[13:42:37.620] dumping /tmp/cores/postgres.7656.core for /tmp/cirrus-ci-build/build/tmp_install/usr/local/pgsql/bin/postgres
[13:42:37.749] [New LWP 101860]
[13:42:37.831] Core was generated by `postgres: primary: checkpointer '.
[13:42:37.831] Program terminated with signal SIGABRT, Aborted.
[13:42:37.831] Sent by thr_kill() from pid 7656 and user 1003.
[13:42:37.831] #0  0x000000082c0f941a in thr_kill () from /lib/libc.so.7
[13:42:37.860] 
[13:42:37.860] Thread 1 (LWP 101860):
[13:42:37.860] #0  0x000000082c0f941a in thr_kill () from /lib/libc.so.7
[13:42:37.860] No symbol table info available.
[13:42:37.860] #1  0x000000082c072e64 in raise () from /lib/libc.so.7
[13:42:37.860] No symbol table info available.
[13:42:37.860] #2  0x000000082c1236f9 in abort () from /lib/libc.so.7
[13:42:37.860] No symbol table info available.
[13:42:37.860] #3  0x0000000000ab2125 in ExceptionalCondition (conditionName=0x340512 "!pgStatLocal.shmem->is_shutdown", fileName=<optimized out>, lineNumber=lineNumber@entry=746) at ../src/backend/utils/error/assert.c:66
[13:42:37.860] No locals.
[13:42:37.860] #4  0x000000000096bcd4 in pgstat_report_stat (force=true) at ../src/backend/utils/activity/pgstat.c:746
[13:42:37.860]         pending_since = 0
[13:42:37.860]         last_flush = 794496967784484
[13:42:37.860]         now = <optimized out>
[13:42:37.860]         partial_flush = <optimized out>
[13:42:37.860]         nowait = <optimized out>
[13:42:37.860] #5  0x000000000096bef9 in pgstat_shutdown_hook (code=<optimized out>, arg=<optimized out>) at ../src/backend/utils/activity/pgstat.c:616
[13:42:37.860] No locals.
[13:42:37.860] #6  0x00000000009221b1 in shmem_exit (code=code@entry=0) at ../src/backend/storage/ipc/ipc.c:243
[13:42:37.860] No locals.
[13:42:37.860] #7  0x00000000009220a8 in proc_exit_prepare (code=101860, code@entry=0) at ../src/backend/storage/ipc/ipc.c:198
[13:42:37.860] No locals.
[13:42:37.860] #8  0x0000000000921fef in proc_exit (code=code@entry=0) at ../src/backend/storage/ipc/ipc.c:111
[13:42:37.860] No locals.
[13:42:37.860] #9  0x00000000008a265e in CheckpointerMain (startup_data=<optimized out>, startup_data_len=<optimized out>) at ../src/backend/postmaster/checkpointer.c:630
[13:42:37.860]         local_sigjmp_buf = {{_sjb = {9052188, 34925197376, 34925197368, 34925197536, 12666224, 11, 1, 35301277696, 895, -2147815357, -1, 34359738369}}}
[13:42:37.860]         checkpointer_context = <optimized out>
[13:42:37.860] #10 0x00000000008a35a5 in postmaster_child_launch (child_type=child_type@entry=B_CHECKPOINTER, child_slot=56, startup_data=startup_data@entry=0x0, startup_data_len=startup_data_len@entry=0, client_sock=client_sock@entry=0x0) at ../src/backend/postmaster/launch_backend.c:274
[13:42:37.860]         pid = <optimized out>
[13:42:37.860] #11 0x00000000008a61a7 in StartChildProcess (type=type@entry=B_CHECKPOINTER) at ../src/backend/postmaster/postmaster.c:3905
[13:42:37.860]         pmchild = 0x0
[13:42:37.860]         pid = <optimized out>
[13:42:37.860] #12 0x00000000008a5d16 in PostmasterMain (argc=argc@entry=4, argv=argv@entry=0x821b43e68) at ../src/backend/postmaster/postmaster.c:1371
[13:42:37.860]         userDoption = <optimized out>
[13:42:37.860]         listen_addr_saved = false
[13:42:37.860]         output_config_variable = <optimized out>
[13:42:37.860]         opt = <optimized out>
[13:42:37.860]         status = <optimized out>
[13:42:37.860] #13 0x00000000007da2e0 in main (argc=4, argv=0x821b43e68) at ../src/backend/main/main.c:230
[13:42:37.860]         do_check_root = <optimized out>
[13:42:37.860]         dispatch_option = DISPATCH_POSTMASTER
[13:42:37.870]

  53. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-03-05T17:26:40Z

    Hi,

On Wed, Mar 05, 2025 at 09:18:16AM -0500, Andres Freund wrote:
> Hi,
> 
> On 2025-03-05 13:03:07 +0000, Bertrand Drouvot wrote:
> > But yeah, if 0002 in [1] does not go in, then your concern is valid, so adding
> > the extra check in the attached.
> 
> This crashes in cfbot:
> 
> https://cirrus-ci.com/task/5111872610893824
> 

Thanks for the report! I usually always run a make check-world locally and also
launch the CI tests on my github repo before submitting patches. This time,
that was a one line change (as compared to v13), so confident enough I did not
trigger those tests. Murphy's Law I guess ;-)

So yeah, back to the issue, we have to pay more attention for the WAL stats
because pgWalUsage is "incremented" without any check with pgstat_tracks_backend_bktype()
(that's not the case for the IO stats where the counters are incremented taking
into account pgstat_tracks_backend_bktype()).

So for the moment, in the attached I "just" add a pgstat_tracks_backend_bktype()
check in pgstat_backend_have_pending_cb() but I'm not sure I like it that much...

Will think more about it...

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  54. Re: per backend WAL statistics

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-03-06T08:13:13Z

    Hi,

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> 于2025年3月5日周三 21:03写道：

> Hi,
>
> On Wed, Mar 05, 2025 at 05:45:57PM +0800, Xuneng Zhou wrote:
> > Subject: Clarification Needed on WAL Pending Checks in Patchset
> >
> > Hi,
> >
> > Thank you for the patchset. I’ve spent some time learning and reviewing
> it
> > and have 2 comments.
>
> Thanks for looking at it!
>
> > I noticed that in patches prior to patch 6, the function
> > pgstat_backend_wal_have_pending() was implemented as follows:
> >
> > /*
> >  * To determine whether any WAL activity has occurred since last time,
> not
> >  * only the number of generated WAL records but also the numbers of WAL
> >  * writes and syncs need to be checked. Because even transactions that
> >  * generate no WAL records can write or sync WAL data when flushing the
> >  * data pages.
> >  */
> > static bool
> > pgstat_backend_wal_have_pending(void)
> > {
> >     PgStat_PendingWalStats pending_wal;
> >
> >     pending_wal = PendingBackendStats.pending_wal;
> >
> >     return pgWalUsage.wal_records != prevBackendWalUsage.wal_records ||
> >            pending_wal.wal_write != 0 || pending_wal.wal_sync != 0;
> > }
> >
> > Starting with patch 7, it seems the implementation was simplified to:
> >
> > /*
> >  * To determine whether WAL usage happened.
> >  */
> > static bool
> > pgstat_backend_wal_have_pending(void)
> > {
> >     return pgWalUsage.wal_records != prevBackendWalUsage.wal_records;
> > }
>
> >That's right. This is due to 2421e9a51d2 that removed
> PgStat_PendingWalStats.
>
> > Meanwhile, the cluster-level check in the function
> > pgstat_wal_have_pending_cb() still performs the additional checks:
> >
> > bool
> > pgstat_wal_have_pending_cb(void)
> > {
> >     return pgWalUsage.wal_records != prevWalUsage.wal_records ||
> >            PendingWalStats.wal_write != 0 ||
> >            PendingWalStats.wal_sync != 0;
> > }
>
> Not since 2421e9a51d2. It looks like that you are looking at code prior to
>> 2421e9a51d2.
>
>
> Yeh, my local version is behind the main branch.
>
> > Another one is on:
> > Bertrand Drouvot <bertranddrouvot.pg@gmail.com> 于2025年3月3日周一 18:47写道：
> >
> > > Hi,
> > >
> > > On Mon, Mar 03, 2025 at 09:17:30AM +0000, Bertrand Drouvot wrote:
> > > > hmm, that would work as long as PGSTAT_BACKEND_FLUSH_ALL represents
> > > things
> > > > that need to be called from pgstat_report_wal(). But I think that's
> open
> > > > door for issue should be add a new PGSTAT_BACKEND_FLUSH_XXX where
> XXX is
> > > not
> > > > related to pgstat_report_wal() at all. So, I'm tempted to keep it as
> it
> > > is.
> > >
> > > I just realized that pgstat_flush_backend() is not correct in 0001.
> Indeed
> > > we check:
> > >
> > > "
> > >     if (pg_memory_is_all_zeros(&PendingBackendStats,
> > >                                sizeof(struct PgStat_BackendPending)))
> > >         return false;
> > > "
> > >
> > > but the WAL stats are not part of PgStat_BackendPending... So we only
> check
> > > for IO pending stats here. I'm not sure WAL stats could be non empty
> if IO
> > > stats are but the attached now also takes care of pending WAL stats
> here.
> >
> > I've noticed that here's a function for checking if there are any backend
> > stats waiting for flush:
> > /*
> >  * Check if there are any backend stats waiting for flush.
> >  */
> > bool
> > pgstat_backend_have_pending_cb(void)
> > {
> > return (!pg_memory_is_all_zeros(&PendingBackendStats,
> > sizeof(struct PgStat_BackendPending)));
> > }
>
> That's right.
>>
>> The reason I did not add the extra check there is because I have in mind
>> to
>> replace the pg_memory_is_all_zeros() checks by a new global variable and
>> also replace
>> the checks in pgstat_flush_backend() by a call to
>> pgstat_backend_have_pending_cb()
>> (see 0002 in [1]). It means that all of that would be perfectly clean if
>> 0002 in [1] goes in.
>>
>> But yeah, if 0002 in [1] does not go in, then your concern is valid, so
>> adding
>> the extra check in the attached.
>>
>> Thanks for the review!
>>
>> [1]:
>> https://www.postgresql.org/message-id/Z8WYf1jyy4MwOveQ%40ip-10-97-1-34.eu-west-3.compute.internal
>>
>> Regards,
>
>

> That makes more sense, I'll take a look at thread 1. Separating patches
> helps clarify their purpose, but it may also fragment the overall
> perspective:) Thanks a lot for your explaination!
>


> --
> Bertrand Drouvot
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com
>

  55. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-03-06T10:33:52Z

    Hi,

On Wed, Mar 05, 2025 at 05:26:40PM +0000, Bertrand Drouvot wrote:
> So yeah, back to the issue, we have to pay more attention for the WAL stats
> because pgWalUsage is "incremented" without any check with pgstat_tracks_backend_bktype()
> (that's not the case for the IO stats where the counters are incremented taking
> into account pgstat_tracks_backend_bktype()).
> 
> So for the moment, in the attached I "just" add a pgstat_tracks_backend_bktype()
> check in pgstat_backend_have_pending_cb() but I'm not sure I like it that much...
> 
> Will think more about it...

After giving more thoughts, I think that the way it's currently done makes sense.
There is no need to check pgstat_tracks_backend_bktype() while incrementing
pgWalUsage or to create a "BackendpgWalUsage" (that would be incremented based
on the pgstat_tracks_backend_bktype()).

In pgstat_create_backend() (called based on pgstat_tracks_backend_bktype()):

- PendingBackendStats is initialized to zero
- prevBackendWalUsage is initialized to pgWalUsage

But:

1. PendingBackendStats is "incremented" during IO operations, so that it makes
sense to ensure that pgstat_tracks_backend_bktype() returns true in those functions
(i.e pgstat_count_backend_io_op_time() and pgstat_count_backend_io_op()).

2. prevBackendWalUsage is not incremented, it's just there to compute the delta
with pgWalUsage in pgstat_flush_backend_entry_wal().

pgstat_flush_backend_entry_wal() is only called from pgstat_flush_backend() that
does the pgstat_tracks_backend_bktype() before. And so is 
pgstat_flush_backend_entry_io().

So, I think it's fine the way it's done here. The missing part was in
pgstat_backend_have_pending_cb() which has been fixed.

But this missing part was already there for the IO stats. It just not manifested
yet maybe because PendingBackendStats was full of zeroes by "luck".

Indeed, there is no reason for pgstat_backend_have_pending_cb() to return true if 
pgstat_tracks_backend_bktype() is not satisfied.

So it deserves a dedicated patch to fix this already existing issue: 0001 attached.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  56. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-03-07T05:42:13Z

    On Thu, Mar 06, 2025 at 10:33:52AM +0000, Bertrand Drouvot wrote:
> Indeed, there is no reason for pgstat_backend_have_pending_cb() to return true if 
> pgstat_tracks_backend_bktype() is not satisfied.
> 
> So it deserves a dedicated patch to fix this already existing issue:
> 0001 attached.

 pgstat_backend_have_pending_cb(void)
 {
-    return (!pg_memory_is_all_zeros(&PendingBackendStats,
-                                    sizeof(struct PgStat_BackendPending)));
+    if (!pgstat_tracks_backend_bktype(MyBackendType))
+        return false;
+    else
+        return (!pg_memory_is_all_zeros(&PendingBackendStats,
+                                        sizeof(struct PgStat_BackendPending)));

So, if I understand your point correctly, it is not a problem on HEAD
because we are never going to update PendingBackendStats in the
checkpointer as pgstat_count_backend_io_op[_time]() blocks any attempt
to do so.  However, it becomes a problem with the WAL portion patch
because of the dependency to pgWalUsage which may be updated by the
checkpointer and the pending callback would happily report true in
this case.  It would also become a problem if we add in backend stats
a different portion that depends on something external.

An extra check based on pgstat_tracks_backend_bktype() makes sense in
pgstat_backend_have_pending_cb(), yes, forcing the hand of the stats
reports to not do stupid things in a process where we should not
report stats.  Good catch from the sanity check in
pgstat_report_stat(), even if this is only a problem once we begin to
use something else than PendingBackendStats for the pending stats.
This has also the merit of making pgstat_report_stat() do less work.
--
Michael

  57. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-03-07T08:33:04Z

    Hi,

On Fri, Mar 07, 2025 at 02:42:13PM +0900, Michael Paquier wrote:
> On Thu, Mar 06, 2025 at 10:33:52AM +0000, Bertrand Drouvot wrote:
> > Indeed, there is no reason for pgstat_backend_have_pending_cb() to return true if 
> > pgstat_tracks_backend_bktype() is not satisfied.
> > 
> > So it deserves a dedicated patch to fix this already existing issue:
> > 0001 attached.
> 
>  pgstat_backend_have_pending_cb(void)
>  {
> -    return (!pg_memory_is_all_zeros(&PendingBackendStats,
> -                                    sizeof(struct PgStat_BackendPending)));
> +    if (!pgstat_tracks_backend_bktype(MyBackendType))
> +        return false;
> +    else
> +        return (!pg_memory_is_all_zeros(&PendingBackendStats,
> +                                        sizeof(struct PgStat_BackendPending)));
> 
> So, if I understand your point correctly, it is not a problem on HEAD
> because we are never going to update PendingBackendStats in the
> checkpointer as pgstat_count_backend_io_op[_time]() blocks any attempt
> to do so.

I think this is wrong on HEAD because we initialize PendingBackendStats to 
zeros in pgstat_create_backend() based on the backend type (pgstat_tracks_backend_bktype()).

But when it's time to flush, then pgstat_backend_have_pending_cb() checks
for zeros in PendingBackendStats *without* any check on the backend type.

I think the issue is "masked" on HEAD because PendingBackendStats is
probably automatically initialized with zeros (as being a static variable at
file scope).

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  58. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-03-08T01:57:38Z

    On Fri, Mar 07, 2025 at 08:33:04AM +0000, Bertrand Drouvot wrote:
> But when it's time to flush, then pgstat_backend_have_pending_cb() checks
> for zeros in PendingBackendStats *without* any check on the backend type.
> 
> I think the issue is "masked" on HEAD because PendingBackendStats is
> probably automatically initialized with zeros (as being a static variable at
> file scope).

If this weren't true, we would have a lot of problems in more places
than this one.  It does not hurt to add an initialization at the top
of pgstat_backend.c for PendingBackendStats, to document the
intention, while we're on it.

Did both things, and applied the result.
--
Michael

  59. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-03-08T07:53:04Z

    Hi,

On Sat, Mar 08, 2025 at 10:57:38AM +0900, Michael Paquier wrote:
> On Fri, Mar 07, 2025 at 08:33:04AM +0000, Bertrand Drouvot wrote:
> > But when it's time to flush, then pgstat_backend_have_pending_cb() checks
> > for zeros in PendingBackendStats *without* any check on the backend type.
> > 
> > I think the issue is "masked" on HEAD because PendingBackendStats is
> > probably automatically initialized with zeros (as being a static variable at
> > file scope).
> 
> If this weren't true, we would have a lot of problems in more places
> than this one.

Yeah I fully agree and I think that was fine. I just added "probably" as 
cautious wording, as the "We assume this initializes to zeroes" comments
that have been removed in 07e9e28b56d and 88f5ebbbee3 for example.

> It does not hurt to add an initialization at the top
> of pgstat_backend.c for PendingBackendStats, to document the
> intention, while we're on it.

-static PgStat_BackendPending PendingBackendStats;
+static PgStat_BackendPending PendingBackendStats = {0};

Not sure about this change: I think that that would not always work should
PgStat_BackendPending contains padding. I mean there is no guarantee that would
initialize padding bytes to zeroes (if any).

That would not be an issue should we only access the struct
fields in the code, but that's not the case as we're making use of
pg_memory_is_all_zeros() on it.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  60. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-03-10T07:46:53Z

    On Sat, Mar 08, 2025 at 07:53:04AM +0000, Bertrand Drouvot wrote:
> That would not be an issue should we only access the struct
> fields in the code, but that's not the case as we're making use of
> pg_memory_is_all_zeros() on it.

It does not hurt to keep it as it is, honestly.

I've reviewed the last patch of the series, and noticed a couple of 
inconsistent comments across it, and some indentation issue.
 
@@ -199,7 +258,8 @@ pgstat_flush_backend(bool nowait, bits32 flags)
         return false;
 
     if (pg_memory_is_all_zeros(&PendingBackendStats,
-                               sizeof(struct PgStat_BackendPending)))
+                               sizeof(struct PgStat_BackendPending))
+        && !pgstat_backend_wal_have_pending())
         return false;

I have one issue with pgstat_flush_backend() and the early exit check
done here.  If for example we use FLUSH_WAL but there is some IO data
pending, we would lock the stats entry for nothing.  We could also
return true even if there is no pending WAL data if the lock could not
be taken, which would be incorrect because there was no data to flush
to begin with.  I think that this should be adjusted so as we limit
the entry lock depending on the flags given in input, like in the
attached.

Thoughts?
--
Michael

  61. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-03-10T11:52:26Z

    Hi,

On Mon, Mar 10, 2025 at 04:46:53PM +0900, Michael Paquier wrote:
> On Sat, Mar 08, 2025 at 07:53:04AM +0000, Bertrand Drouvot wrote:
> > That would not be an issue should we only access the struct
> > fields in the code, but that's not the case as we're making use of
> > pg_memory_is_all_zeros() on it.
> 
> It does not hurt to keep it as it is, honestly.

I believe that's worse than before actually. Before padding bytes would "probably"
be set to zeros while now it's certainly not always the case. I think that
we already removed this (see comments === 4 in [1]).

> I've reviewed the last patch of the series

Thanks!

> and noticed a couple of 
> inconsistent comments across it, and some indentation issue.

I think I ran pgindent though. Anyway, thanks for fixing those!

> @@ -199,7 +258,8 @@ pgstat_flush_backend(bool nowait, bits32 flags)
>          return false;
>  
>      if (pg_memory_is_all_zeros(&PendingBackendStats,
> -                               sizeof(struct PgStat_BackendPending)))
> +                               sizeof(struct PgStat_BackendPending))
> +        && !pgstat_backend_wal_have_pending())
>          return false;
> 
> I have one issue with pgstat_flush_backend() and the early exit check
> done here.  If for example we use FLUSH_WAL but there is some IO data
> pending, we would lock the stats entry for nothing.  We could also
> return true even if there is no pending WAL data if the lock could not
> be taken, which would be incorrect because there was no data to flush
> to begin with.  I think that this should be adjusted so as we limit
> the entry lock depending on the flags given in input, like in the
> attached.

Yeah I agree this needs to be improved, thanks!

+       /* Some IO data pending? */
+       if ((flags & PGSTAT_BACKEND_FLUSH_IO) &&
+               !pg_memory_is_all_zeros(&PendingBackendStats,
+                                                               sizeof(struct PgStat_BackendPending)))
+               has_pending_data = true;

if PgStat_BackendPending contains more than pending_io in the future, then
that would check for zeros in a too large memory region.

I think it's better to check for:

	if (pg_memory_is_all_zeros(&PendingBackendStats.pending_io,
							   sizeof(struct PgStat_PendingIO)))

like in the attached. Or check on "backend_has_iostats" (if 0002 in [2] goes in).

+       /* Some WAL data pending? */
+       if ((flags & PGSTAT_BACKEND_FLUSH_WAL) &&
+               pgstat_backend_wal_have_pending())
+               has_pending_data = true;

I think we can use "else if" here (done in the attached) as it's not needed if
has_pending_data is already set to true.

That's the only 2 changes done in the attached as compared to the previous
version.

Regards,

[1]: https://www.postgresql.org/message-id/Z44vMD/rALy8pfVE%40ip-10-97-1-34.eu-west-3.compute.internal
[2]: https://www.postgresql.org/message-id/Z8WYf1jyy4MwOveQ%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  62. Re: per backend WAL statistics

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2025-03-10T12:08:49Z

    Hi,

Thank you for working on this!

I just started reading the code and have a couple of questions.

I think that every time we flush IO or WAL stats, we want(?) to flush
backend stats as well, so would it make sense to move
pgstat_flush_backend() calls to inside of pgstat_flush_io() and
pgstat_wal_flush_cb()? I see that backend statistics are not collected
for some of the backend types but that is already checked in the
pgstat_flush_backend() with pgstat_tracks_backend_bktype().

Also, is there a chance that wal_bytes gets incremented without
wal_records getting incremented? I searched the code and did not find
any example of that but I just wanted to be sure. If there is a case
like that, then pgstat_backend_wal_have_pending() needs to check
wal_bytes instead of wal_records.

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

  63. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-03-10T14:43:17Z

    Hi,

On Mon, Mar 10, 2025 at 03:08:49PM +0300, Nazir Bilal Yavuz wrote:
> Hi,
> 
> Thank you for working on this!
> 
> I just started reading the code and have a couple of questions.

Thanks for looking at it!

> I think that every time we flush IO or WAL stats, we want(?) to flush
> backend stats as well,

Yeah, I think that's happening anyway.

> so would it make sense to move
> pgstat_flush_backend() calls to inside of pgstat_flush_io() and
> pgstat_wal_flush_cb()?

I don't think so because pgstat_flush_backend() still needs to be called by the
pgstat_backend_flush_cb() (i.e flush_static_cb) callback (I mean I think this
makes sense to keep this callback around and that it does "really" something).

So for example, for the WAL case, that would mean the backend WAL stats would be
flushed twice: one time from pgstat_wal_flush_cb() (i.e flush_static_cb) callback
and one time from the pgstat_backend_flush_cb() (another flush_static_cb) callback.

I think it's better to keep them separate and reason as they are distinct
types of stats (which they really are). I think we had the same kind of reasoning
while working on [1].

> I see that backend statistics are not collected
> for some of the backend types but that is already checked in the
> pgstat_flush_backend() with pgstat_tracks_backend_bktype().

Sorry, I don't get it. Do you have a question around that?

> Also, is there a chance that wal_bytes gets incremented without
> wal_records getting incremented? I searched the code and did not find
> any example of that but I just wanted to be sure. If there is a case
> like that, then pgstat_backend_wal_have_pending() needs to check
> wal_bytes instead of wal_records.

I think that's fine. That's also how pgstat_wal_have_pending_cb() has been 
re-factored in 2421e9a51d2.

[1]: https://www.postgresql.org/message-id/flat/Z0QjeIkwC0HNI16K%40ip-10-97-1-34.eu-west-3.compute.internal#16762c1767ffb168318e7c3734fa5f64

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  64. Re: per backend WAL statistics

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2025-03-10T16:07:05Z

    Hi,

On Mon, 10 Mar 2025 at 17:43, Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
> On Mon, Mar 10, 2025 at 03:08:49PM +0300, Nazir Bilal Yavuz wrote:
>
> > I think that every time we flush IO or WAL stats, we want(?) to flush
> > backend stats as well,
>
> Yeah, I think that's happening anyway.
>
> > so would it make sense to move
> > pgstat_flush_backend() calls to inside of pgstat_flush_io() and
> > pgstat_wal_flush_cb()?
>
> I don't think so because pgstat_flush_backend() still needs to be called by the
> pgstat_backend_flush_cb() (i.e flush_static_cb) callback (I mean I think this
> makes sense to keep this callback around and that it does "really" something).
>
> So for example, for the WAL case, that would mean the backend WAL stats would be
> flushed twice: one time from pgstat_wal_flush_cb() (i.e flush_static_cb) callback
> and one time from the pgstat_backend_flush_cb() (another flush_static_cb) callback.
>
> I think it's better to keep them separate and reason as they are distinct
> types of stats (which they really are). I think we had the same kind of reasoning
> while working on [1].

I got it, that makes sense. Thanks for the explanation.

> > I see that backend statistics are not collected
> > for some of the backend types but that is already checked in the
> > pgstat_flush_backend() with pgstat_tracks_backend_bktype().
>
> Sorry, I don't get it. Do you have a question around that?

Sorry for the confusion. I did not think of the explanation that you
did above. I was thinking that we do not want to call
pgstat_flush_backend(.., PGSTAT_BACKEND_FLUSH_IO) for the (let's say)
checkpointer as its backend statistics are not collected and that is
the reason why we do not want to put pgstat_flush_backend() inside of
pgstat_flush_io(). Your explanation made it clear now, no other
questions.

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

  65. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-03-11T00:06:27Z

    On Mon, Mar 10, 2025 at 11:52:26AM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Mon, Mar 10, 2025 at 04:46:53PM +0900, Michael Paquier wrote:
> > On Sat, Mar 08, 2025 at 07:53:04AM +0000, Bertrand Drouvot wrote:
> > > That would not be an issue should we only access the struct
> > > fields in the code, but that's not the case as we're making use of
> > > pg_memory_is_all_zeros() on it.
> > 
> > It does not hurt to keep it as it is, honestly.
> 
> I believe that's worse than before actually. Before padding bytes would "probably"
> be set to zeros while now it's certainly not always the case. I think that
> we already removed this (see comments === 4 in [1]).

We still apply the memset(), and the initialization is actually the
same.

> I think it's better to check for:
> 
>     if (pg_memory_is_all_zeros(&PendingBackendStats.pending_io,
>                                sizeof(struct PgStat_PendingIO)))
> 
> like in the attached. Or check on "backend_has_iostats" (if 0002 in [2] goes in).

Yes, restricting this check to apply on the PgStat_PendingIO makes
sense.

> I think we can use "else if" here (done in the attached) as it's not needed if
> has_pending_data is already set to true.

Still the blocks with the comments become a bit weird if formulated
this way.  Kept this one the same as v17.

And I guess that we're OK here, so applied.  That should be the last
one.
--
Michael

  66. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-03-11T06:23:43Z

    Hi,

On Tue, Mar 11, 2025 at 09:06:27AM +0900, Michael Paquier wrote:
> On Mon, Mar 10, 2025 at 11:52:26AM +0000, Bertrand Drouvot wrote:
> > Hi,
> > 
> > On Mon, Mar 10, 2025 at 04:46:53PM +0900, Michael Paquier wrote:
> > > On Sat, Mar 08, 2025 at 07:53:04AM +0000, Bertrand Drouvot wrote:
> > > > That would not be an issue should we only access the struct
> > > > fields in the code, but that's not the case as we're making use of
> > > > pg_memory_is_all_zeros() on it.
> > > 
> > > It does not hurt to keep it as it is, honestly.
> > 
> > I believe that's worse than before actually. Before padding bytes would "probably"
> > be set to zeros while now it's certainly not always the case. I think that
> > we already removed this (see comments === 4 in [1]).
> 
> We still apply the memset(), and the initialization is actually the
> same.

Yeah currently there is no issues: there is no padding in the struct and memset()
is done.

That said, memset() is done only if pgstat_tracks_backend_bktype() returns
true (i.e if pgstat_create_backend() is called).

That means that if, in the future, the struct is modified in such a way that
padding is added, then we could end up with non zeros padding bytes for the
backends for which pgstat_tracks_backend_bktype() returns false.

I think that could lead to racy conditions (even if, for the moment, I think that
all is fine as the other pgstat_tracks_backend_bktype() calls should protect us).

> And I guess that we're OK here,

Yup.

> so applied.

Thanks!

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  67. Re: per backend WAL statistics

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-03-12T03:14:24Z

    Michael Paquier <michael@paquier.xyz> writes:
> And I guess that we're OK here, so applied.  That should be the last
> one.

Quite a few buildfarm members are not happy about the initialization
that 9a8dd2c5a added to PendingBackendStats.  For instance [1]:

gcc -std=gnu99 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -O2 -I. -I. -I../../../../src/include  -D_GNU_SOURCE -I/usr/include/libxml2   -c -o pgstat_backend.o pgstat_backend.c
pgstat_backend.c:39:1: warning: missing braces around initializer [-Wmissing-braces]
 static PgStat_BackendPending PendingBackendStats = {0};
 ^
pgstat_backend.c:39:1: warning: (near initialization for \342\200\230PendingBackendStats.pending_io\342\200\231) [-Wmissing-braces]

I guess that more than one level of braces is needed for this to
be fully correct?  Similar from ayu, batfish, boa, buri, demoiselle,
dhole, rhinoceros, shelduck, siskin.

			regards, tom lane

[1] https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=arowana&dt=2025-03-11%2004%3A59%3A16&stg=build

  68. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-03-12T05:37:11Z

    Hi,

On Tue, Mar 11, 2025 at 11:14:24PM -0400, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
> > And I guess that we're OK here, so applied.  That should be the last
> > one.
> 
> Quite a few buildfarm members are not happy about the initialization
> that 9a8dd2c5a added to PendingBackendStats.  For instance [1]:
> 
> gcc -std=gnu99 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -O2 -I. -I. -I../../../../src/include  -D_GNU_SOURCE -I/usr/include/libxml2   -c -o pgstat_backend.o pgstat_backend.c
> pgstat_backend.c:39:1: warning: missing braces around initializer [-Wmissing-braces]
>  static PgStat_BackendPending PendingBackendStats = {0};
>  ^
> pgstat_backend.c:39:1: warning: (near initialization for \342\200\230PendingBackendStats.pending_io\342\200\231) [-Wmissing-braces]
> 
> I guess that more than one level of braces is needed for this to
> be fully correct?

Thanks for the report! I think that it's better to remove the PendingBackendStats
initializer (instead of adding extra braces). The reason is that I'm concerned
about padding bytes (that could be added to the struct in the future) not being
zeroed (see [1]).

Done that way in the attached.

[1]: https://www.postgresql.org/message-id/Z8/W73%2BHVo%2B/pKHZ%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  69. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-03-12T11:32:46Z

    On Wed, Mar 12, 2025 at 05:37:11AM +0000, Bertrand Drouvot wrote:
> Thanks for the report! I think that it's better to remove the PendingBackendStats
> initializer (instead of adding extra braces). The reason is that I'm concerned
> about padding bytes (that could be added to the struct in the future) not being
> zeroed (see [1]).

Okay.  I am going to remove this initialization in a couple of minutes
as that's more annoying than I thought.
--
Michael

  70. Re: per backend WAL statistics

    Alexander Law <exclusion@gmail.com> — 2025-03-28T19:00:00Z

    Hello Michael,

11.03.2025 02:06, Michael Paquier wrote:
> And I guess that we're OK here, so applied.  That should be the last
> one.

Please try the following query:
BEGIN;
SET LOCAL stats_fetch_consistency = snapshot;
SELECT * FROM pg_stat_get_backend_wal(pg_backend_pid());

with sanitizers (or under Valgrind). When I run it, I get:
2025-03-28 18:38:08.259 UTC [3415399] LOG:  statement: SELECT * FROM pg_stat_get_backend_wal(pg_backend_pid());
=================================================================
==3415399==ERROR: AddressSanitizer: heap-use-after-free on address 0x53100003c83c at pc 0x556e3d2d9967 bp 0x7ffda3cd2350 
sp 0x7ffda3cd2340
READ of size 4 at 0x53100003c83c thread T0
     #0 0x556e3d2d9966 in pgstat_fetch_stat_backend_by_pid .../src/backend/utils/activity/pgstat_backend.c:136
     #1 0x556e3d53b671 in pg_stat_get_backend_wal .../src/backend/utils/adt/pgstatfuncs.c:1673
     #2 0x556e3cb14045 in ExecMakeTableFunctionResult .../src/backend/executor/execSRF.c:234
     #3 0x556e3cb6c0fd in FunctionNext .../src/backend/executor/nodeFunctionscan.c:94
     #4 0x556e3cb171d2 in ExecScanFetch ../../../src/include/executor/execScan.h:126
     #5 0x556e3cb171d2 in ExecScanExtended ../../../src/include/executor/execScan.h:170
     #6 0x556e3cb171d2 in ExecScan .../src/backend/executor/execScan.c:59
     #7 0x556e3cb6bbf7 in ExecFunctionScan .../src/backend/executor/nodeFunctionscan.c:269
     #8 0x556e3cb0aba9 in ExecProcNodeFirst .../src/backend/executor/execProcnode.c:469
...

Reproduced starting from 76def4cdd.

Best regards,
Alexander Lakhin
Neon (https://neon.tech)

  71. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-03-28T22:14:16Z

    On Fri, Mar 28, 2025 at 09:00:00PM +0200, Alexander Lakhin wrote:
> Please try the following query:
> BEGIN;
> SET LOCAL stats_fetch_consistency = snapshot;
> SELECT * FROM pg_stat_get_backend_wal(pg_backend_pid());
> 
> with sanitizers (or under Valgrind). When I run it, I get:
> 2025-03-28 18:38:08.259 UTC [3415399] LOG:  statement: SELECT * FROM pg_stat_get_backend_wal(pg_backend_pid());
> =================================================================
> ==3415399==ERROR: AddressSanitizer: heap-use-after-free on address
> 0x53100003c83c at pc 0x556e3d2d9967 bp 0x7ffda3cd2350 sp 0x7ffda3cd2340
> READ of size 4 at 0x53100003c83c thread T0
>     #0 0x556e3d2d9966 in pgstat_fetch_stat_backend_by_pid .../src/backend/utils/activity/pgstat_backend.c:136
>     #1 0x556e3d53b671 in pg_stat_get_backend_wal .../src/backend/utils/adt/pgstatfuncs.c:1673
>     #2 0x556e3cb14045 in ExecMakeTableFunctionResult .../src/backend/executor/execSRF.c:234
>     #3 0x556e3cb6c0fd in FunctionNext .../src/backend/executor/nodeFunctionscan.c:94
>     #4 0x556e3cb171d2 in ExecScanFetch ../../../src/include/executor/execScan.h:126
>     #5 0x556e3cb171d2 in ExecScanExtended ../../../src/include/executor/execScan.h:170
>     #6 0x556e3cb171d2 in ExecScan .../src/backend/executor/execScan.c:59
>     #7 0x556e3cb6bbf7 in ExecFunctionScan .../src/backend/executor/nodeFunctionscan.c:269
>     #8 0x556e3cb0aba9 in ExecProcNodeFirst .../src/backend/executor/execProcnode.c:469
> ...
> 
> Reproduced starting from 76def4cdd.

Thanks for the report.  I have added an open item to not lose track of
this issue, and will get back to it when I can.
--
Michael

  72. Re: per backend WAL statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-03-31T07:42:19Z

    Hi,

On Sat, Mar 29, 2025 at 07:14:16AM +0900, Michael Paquier wrote:
> On Fri, Mar 28, 2025 at 09:00:00PM +0200, Alexander Lakhin wrote:
> > Please try the following query:
> > BEGIN;
> > SET LOCAL stats_fetch_consistency = snapshot;
> > SELECT * FROM pg_stat_get_backend_wal(pg_backend_pid());

Thanks for the report! I'm able to reproduce it on my side. The issue can
also be triggered with pg_stat_get_backend_io().

The issue is that in pgstat_fetch_stat_backend_by_pid() (and with
stats_fetch_consistency set to snapshot) a call to
pgstat_clear_backend_activity_snapshot() is done:

#0  __memset_evex_unaligned_erms () at ../sysdeps/x86_64/multiarch/memset-vec-unaligned-erms.S:250
#1  0x0000000001833bf2 in wipe_mem (ptr=0x632000018800, size=80800) at ../../../../src/include/utils/memdebug.h:42
#2  0x0000000001834c51 in AllocSetReset (context=0x619000003c80) at aset.c:586
#3  0x000000000184f32d in MemoryContextResetOnly (context=0x619000003c80) at mcxt.c:419
#4  0x0000000001834ede in AllocSetDelete (context=0x619000003c80) at aset.c:636
#5  0x000000000184f79b in MemoryContextDeleteOnly (context=0x619000003c80) at mcxt.c:528
#6  0x000000000184f5a9 in MemoryContextDelete (context=0x619000003c80) at mcxt.c:482
#7  0x0000000001361e84 in pgstat_clear_backend_activity_snapshot () at backend_status.c:541
#8  0x0000000001367f08 in pgstat_clear_snapshot () at pgstat.c:943
#9  0x0000000001368ac3 in pgstat_prep_snapshot () at pgstat.c:1121
#10 0x00000000013680b9 in pgstat_fetch_entry (kind=6, dboid=0, objid=0) at pgstat.c:961
#11 0x000000000136dd05 in pgstat_fetch_stat_backend (procNumber=0) at pgstat_backend.c:94
#12 0x000000000136de7d in pgstat_fetch_stat_backend_by_pid (pid=3294022, bktype=0x0) at pgstat_backend.c:136

*before* we check for "beentry->st_procpid != pid".

I think we can simply move the pgstat_fetch_stat_backend() call at the end
of pgstat_fetch_stat_backend_by_pid(), like in the attached. With this in place
the issue is fixed on my side.

Thoughts?

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  73. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-03-31T11:43:22Z

    > On Mar 31, 2025, at 16:42, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
> I think we can simply move the pgstat_fetch_stat_backend() call at the end
> of pgstat_fetch_stat_backend_by_pid(), like in the attached. With this in place
> the issue is fixed on my side.

Thanks for the patch, I’ll check all that next week.
--
Michael

  74. Re: per backend WAL statistics

    Michael Paquier <michael@paquier.xyz> — 2025-04-07T00:54:46Z

    On Mon, Mar 31, 2025 at 07:42:19AM +0000, Bertrand Drouvot wrote:
> I think we can simply move the pgstat_fetch_stat_backend() call at the end
> of pgstat_fetch_stat_backend_by_pid(), like in the attached. With this in place
> the issue is fixed on my side.
> 
> Thoughts?

Confirmed.  I agree that it is simpler to move all the accesses to
beentry before attempting to retrieve the pgstats entry.

One thing that itched me a bit in the patch is that we would set
bktype even if we don't have a pgstats entry.  The two callers of
pgstat_fetch_stat_backend_by_pid() return tuples full of NULLs and
zeros in this case, discarding the backend type automatically, but
let's keep the API consistent and set the value to B_INVALID if
pgstat_fetch_stat_backend() returns NULL.

I have added a comment warning about not accessing beentry when
fetching the backend pgstats entry, and applied the result.  Thanks
for the report, Alexander, and for the patch, Bertrand.
--
Michael