Horton

Thread

Commits

  1. Rework handling of pending data for backend statistics

  2. Rename some pgstats callbacks related to flush of entries

  3. Relax regression test for fsync check of backend-level stats

  4. Add backend-level statistics to pgstats

  5. Extract logic filling pg_stat_get_io()'s tuplestore into its own routine

  6. Tweak some comments related to variable-numbered stats in pgstat.c

  1. per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-09-02T14:55:52Z

    Hi hackers,

Please find attached a patch to implement $SUBJECT.

While pg_stat_io provides cluster-wide I/O statistics, this patch adds a new
pg_my_stat_io view to display "my" backend I/O statistics and a new
pg_stat_get_backend_io() function to retrieve the I/O statistics for a given
backend pid.

By having the per backend level of granularity, one could for example identify
which running backend is responsible for most of the reads, most of the extends
and so on... The pg_my_stat_io view could also be useful to check the
impact on the I/O made by some operations, queries,... in the current session.

Some remarks:

- it is split in 2 sub patches: 0001 introducing the necessary changes to provide
the pg_my_stat_io view and 0002 to add the pg_stat_get_backend_io() function.
- the idea of having per backend I/O statistics has already been mentioned in
[1] by Andres.

Some implementation choices:

- The KIND_IO stats are still "fixed amount" ones as the maximum number of
backend is fixed.
- The statistics snapshot is made for the global stats (the aggregated ones) and
for my backend stats. The snapshot is not build for all the backend stats (that
could be memory expensive depending on the number of max connections and given
the fact that PgStat_IO is 16KB long).
- The above point means that pg_stat_get_backend_io() behaves as if
stats_fetch_consistency is set to none (each execution re-fetches counters
from shared memory).
- The above 2 points are also the reasons why the pg_my_stat_io view has been
added (as its results takes care of the stats_fetch_consistency setting). I think
that makes sense to rely on it in that case, while I'm not sure that would make
a lot of sense to retrieve other's backend I/O stats and taking care of
stats_fetch_consistency.


[1]: https://www.postgresql.org/message-id/20230309003438.rectf7xo7pw5t5cj%40awork3.anarazel.de

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 I/O statistics

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-09-03T06:37:49Z

    At Mon, 2 Sep 2024 14:55:52 +0000, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote in 
> Hi hackers,
> 
> Please find attached a patch to implement $SUBJECT.
> 
> While pg_stat_io provides cluster-wide I/O statistics, this patch adds a new
> pg_my_stat_io view to display "my" backend I/O statistics and a new
> pg_stat_get_backend_io() function to retrieve the I/O statistics for a given
> backend pid.
> 
> By having the per backend level of granularity, one could for example identify
> which running backend is responsible for most of the reads, most of the extends
> and so on... The pg_my_stat_io view could also be useful to check the
> impact on the I/O made by some operations, queries,... in the current session.
> 
> Some remarks:
> 
> - it is split in 2 sub patches: 0001 introducing the necessary changes to provide
> the pg_my_stat_io view and 0002 to add the pg_stat_get_backend_io() function.
> - the idea of having per backend I/O statistics has already been mentioned in
> [1] by Andres.
> 
> Some implementation choices:
> 
> - The KIND_IO stats are still "fixed amount" ones as the maximum number of
> backend is fixed.
> - The statistics snapshot is made for the global stats (the aggregated ones) and
> for my backend stats. The snapshot is not build for all the backend stats (that
> could be memory expensive depending on the number of max connections and given
> the fact that PgStat_IO is 16KB long).
> - The above point means that pg_stat_get_backend_io() behaves as if
> stats_fetch_consistency is set to none (each execution re-fetches counters
> from shared memory).
> - The above 2 points are also the reasons why the pg_my_stat_io view has been
> added (as its results takes care of the stats_fetch_consistency setting). I think
> that makes sense to rely on it in that case, while I'm not sure that would make
> a lot of sense to retrieve other's backend I/O stats and taking care of
> stats_fetch_consistency.
> 
> 
> [1]: https://www.postgresql.org/message-id/20230309003438.rectf7xo7pw5t5cj%40awork3.anarazel.de

I'm not sure about the usefulness of having the stats only available
from the current session. Since they are stored in shared memory,
shouldn't we make them accessible to all backends? However, this would
introduce permission considerations and could become complex.

When I first looked at this patch, my initial thought was whether we
should let these stats stay "fixed." The reason why the current
PGSTAT_KIND_IO is fixed is that there is only one global statistics
storage for the entire database. If we have stats for a flexible
number of backends, it would need to be non-fixed, perhaps with the
entry for INVALID_PROC_NUMBER storing the global I/O stats, I
suppose. However, one concern with that approach would be the impact
on performance due to the frequent creation and deletion of stats
entries caused by high turnover of backends.

Just to be clear, the above comments are not meant to oppose the
current implementation approach. They are purely for the sake of
discussing comparisons with other possible approaches.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  3. Re: per backend I/O statistics

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-09-03T07:07:58Z

    At Tue, 03 Sep 2024 15:37:49 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> When I first looked at this patch, my initial thought was whether we
> should let these stats stay "fixed." The reason why the current
> PGSTAT_KIND_IO is fixed is that there is only one global statistics
> storage for the entire database. If we have stats for a flexible
> number of backends, it would need to be non-fixed, perhaps with the
> entry for INVALID_PROC_NUMBER storing the global I/O stats, I
> suppose. However, one concern with that approach would be the impact
> on performance due to the frequent creation and deletion of stats
> entries caused by high turnover of backends.

As an additional benefit of this approach, the client can set a
connection variable, for example, no_backend_iostats to true, or set
its inverse variable to false, to restrict memory usage to only the
required backends.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  4. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-09-03T07:21:23Z

    Hi,

On Tue, Sep 03, 2024 at 03:37:49PM +0900, Kyotaro Horiguchi wrote:
> At Mon, 2 Sep 2024 14:55:52 +0000, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote in 
> > Hi hackers,
> > 
> > Please find attached a patch to implement $SUBJECT.
> > 
> > While pg_stat_io provides cluster-wide I/O statistics, this patch adds a new
> > pg_my_stat_io view to display "my" backend I/O statistics and a new
> > pg_stat_get_backend_io() function to retrieve the I/O statistics for a given
> > backend pid.
> > 
> 
> I'm not sure about the usefulness of having the stats only available
> from the current session. Since they are stored in shared memory,
> shouldn't we make them accessible to all backends?

Thanks for the feedback!

The stats are accessible to all backends thanks to 0002 and the introduction
of the pg_stat_get_backend_io() function.

> However, this would
> introduce permission considerations and could become complex.

Not sure that the data exposed here is sensible enough to consider permission
restriction.

> When I first looked at this patch, my initial thought was whether we
> should let these stats stay "fixed." The reason why the current
> PGSTAT_KIND_IO is fixed is that there is only one global statistics
> storage for the entire database. If we have stats for a flexible
> number of backends, it would need to be non-fixed, perhaps with the
> entry for INVALID_PROC_NUMBER storing the global I/O stats, I
> suppose. However, one concern with that approach would be the impact
> on performance due to the frequent creation and deletion of stats
> entries caused by high turnover of backends.
>

The pros of using the fixed amount are:

- less code change (I think as I did not write the non fixed counterpart)
- probably better performance and less scalabilty impact (in case of high rate
of backends creation/ deletion)

Cons is probably allocating shared memory space that might not be used (
sizeof(PgStat_IO) is 16392 so that could be a concern for a high number of
unused connection). OTOH, if a high number of connections is not used that might
be worth to reduce the max_connections setting.

"Conceptually" speaking, we know what the maximum number of backend is, so I
think that using the fixed amount approach makes sense (somehow I think it can
be compared to PGSTAT_KIND_SLRU which relies on SLRU_NUM_ELEMENTS).

> Just to be clear, the above comments are not meant to oppose the
> current implementation approach. They are purely for the sake of
> discussing comparisons with other possible approaches.

No problem at all, thanks for your feedback and sharing your thoughts!

Regards,

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

  5. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-09-04T04:45:24Z

    Hi,

On Tue, Sep 03, 2024 at 04:07:58PM +0900, Kyotaro Horiguchi wrote:
> At Tue, 03 Sep 2024 15:37:49 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> > When I first looked at this patch, my initial thought was whether we
> > should let these stats stay "fixed." The reason why the current
> > PGSTAT_KIND_IO is fixed is that there is only one global statistics
> > storage for the entire database. If we have stats for a flexible
> > number of backends, it would need to be non-fixed, perhaps with the
> > entry for INVALID_PROC_NUMBER storing the global I/O stats, I
> > suppose. However, one concern with that approach would be the impact
> > on performance due to the frequent creation and deletion of stats
> > entries caused by high turnover of backends.
> 
> As an additional benefit of this approach, the client can set a
> connection variable, for example, no_backend_iostats to true, or set
> its inverse variable to false, to restrict memory usage to only the
> required backends.

Thanks for the feedback!

If we were to add an on/off switch button, I think I'd vote for a global one
instead. Indeed, I see this feature more like an "Administrator" one, where
the administrator wants to be able to find out which session is reponsible of
what (from an I/O point of view): like being able to anwser "which session is
generating this massive amount of reads"?

If we allow each session to disable the feature then the administrator
would lost this ability.

Regards,

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

  6. Re: per backend I/O statistics

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-09-05T13:03:32Z

    On 2024-Sep-03, Bertrand Drouvot wrote:

> Cons is probably allocating shared memory space that might not be used (
> sizeof(PgStat_IO) is 16392 so that could be a concern for a high number of
> unused connection). OTOH, if a high number of connections is not used that might
> be worth to reduce the max_connections setting.

I was surprised by the size you mention so I went to look for the
definition of that struct:

typedef struct PgStat_IO
{
	TimestampTz stat_reset_timestamp;
	PgStat_BktypeIO stats[BACKEND_NUM_TYPES];
} PgStat_IO;

(It would be good to have more than zero comments here BTW)

So what's happening is that struct PgStat_IO stores the data for every
single process type, be it regular backends, backend-like auxiliary
processes, and all other potential postmaster children.  So it seems to
me that storing one of these struct for "my backend stats" is wrong: I
think you should be using PgStat_BktypeIO instead (or maybe another
struct which has a reset timestamp plus BktypeIO, if you care about the
reset time).  That struct is only 1024 bytes long, so it seems much more
reasonable to have one of those per backend.

Another way to think about this might be to say that the B_BACKEND
element of the PgStat_Snapshot.io array should really be spread out to
MaxBackends separate elements.  This would make it more difficult to
obtain a total value accumulating ops done by all backends (since it's
require to sum the values of each backend), but it allows you to obtain
backend-specific values, including those of remote backends rather than
only your own, as Kyotaro suggests.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

  7. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-09-05T14:14:47Z

    Hi,

On Thu, Sep 05, 2024 at 03:03:32PM +0200, Alvaro Herrera wrote:
> On 2024-Sep-03, Bertrand Drouvot wrote:
> 
> > Cons is probably allocating shared memory space that might not be used (
> > sizeof(PgStat_IO) is 16392 so that could be a concern for a high number of
> > unused connection). OTOH, if a high number of connections is not used that might
> > be worth to reduce the max_connections setting.
> 
> I was surprised by the size you mention so I went to look for the
> definition of that struct:
>

Thanks for looking at it!

 
> typedef struct PgStat_IO
> {
> 	TimestampTz stat_reset_timestamp;
> 	PgStat_BktypeIO stats[BACKEND_NUM_TYPES];
> } PgStat_IO;
> 
> (It would be good to have more than zero comments here BTW)
> 
> So what's happening is that struct PgStat_IO stores the data for every
> single process type, be it regular backends, backend-like auxiliary
> processes, and all other potential postmaster children.

Yeap.

> So it seems to
> me that storing one of these struct for "my backend stats" is wrong: I
> think you should be using PgStat_BktypeIO instead (or maybe another
> struct which has a reset timestamp plus BktypeIO, if you care about the
> reset time).  That struct is only 1024 bytes long, so it seems much more
> reasonable to have one of those per backend.

Yeah, that could be an area of improvement. Thanks, I'll look at it.
Currently the filtering is done when retrieving the per backend stats but better
to do it when storing the stats.

> Another way to think about this might be to say that the B_BACKEND
> element of the PgStat_Snapshot.io array should really be spread out to
> MaxBackends separate elements.  This would make it more difficult to
> obtain a total value accumulating ops done by all backends (since it's
> require to sum the values of each backend), but it allows you to obtain
> backend-specific values, including those of remote backends rather than
> only your own, as Kyotaro suggests.
>

One backend can already see the stats of the other backends thanks to the
pg_stat_get_backend_io() function (that takes a backend pid as parameter)
that is introduced in the 0002 sub-patch.

I'll ensure that's still the case with the next version of the patch.

Regards,

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

  8. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-09-06T15:03:17Z

    Hi,

On Thu, Sep 05, 2024 at 02:14:47PM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Thu, Sep 05, 2024 at 03:03:32PM +0200, Alvaro Herrera wrote:
> > So it seems to
> > me that storing one of these struct for "my backend stats" is wrong: I
> > think you should be using PgStat_BktypeIO instead (or maybe another
> > struct which has a reset timestamp plus BktypeIO, if you care about the
> > reset time).  That struct is only 1024 bytes long, so it seems much more
> > reasonable to have one of those per backend.
> 
> Yeah, that could be an area of improvement. Thanks, I'll look at it.
> Currently the filtering is done when retrieving the per backend stats but better
> to do it when storing the stats.

I ended up creating (in v2 attached):

"
typedef struct PgStat_Backend_IO
{
    TimestampTz stat_reset_timestamp;
    BackendType bktype;
    PgStat_BktypeIO stats;
} PgStat_Backend_IO;
"

The stat_reset_timestamp is there so that one knows when a particular backend
had its I/O stats reset. Also the backend type is part of the struct to
be able to filter the stats correctly when we display them.

The struct size is 1040 Bytes and that's much more reasonable than the size
needed for per backend I/O stats in v1 (about 16KB).

> One backend can already see the stats of the other backends thanks to the
> pg_stat_get_backend_io() function (that takes a backend pid as parameter)
> that is introduced in the 0002 sub-patch.

0002 still provides the pg_stat_get_backend_io() function so that one could
get the stats of other backends.

Example:

postgres=# select backend_type,object,context,reads,extends,hits from pg_stat_get_backend_io(3779502);
  backend_type  |    object     |  context  | reads | extends |  hits
----------------+---------------+-----------+-------+---------+--------
 client backend | relation      | bulkread  |     0 |         |      0
 client backend | relation      | bulkwrite |     0 |       0 |      0
 client backend | relation      | normal    |    73 |    2216 | 504674
 client backend | relation      | vacuum    |     0 |       0 |      0
 client backend | temp relation | normal    |     0 |       0 |      0
(5 rows)

could be an individual walsender:

postgres=# select pid, backend_type from pg_stat_activity where backend_type = 'walsender';
   pid   | backend_type
---------+--------------
 3779565 | walsender
(1 row)

postgres=# select backend_type,object,context,reads,hits from pg_stat_get_backend_io(3779565);
 backend_type |    object     |  context  | reads | hits
--------------+---------------+-----------+-------+------
 walsender    | relation      | bulkread  |     0 |    0
 walsender    | relation      | bulkwrite |     0 |    0
 walsender    | relation      | normal    |     6 |   48
 walsender    | relation      | vacuum    |     0 |    0
 walsender    | temp relation | normal    |     0 |    0
(5 rows)

and so on...

Remarks:

- As stated up-thread, the pg_stat_get_backend_io() behaves as if
stats_fetch_consistency is set to none (each execution re-fetches counters
from shared memory). Indeed, the snapshot is not build in each backend to copy
all the others backends stats, as 1/ there is no use case (there is no need to
get others backends I/O statistics while taking care of the stats_fetch_consistency)
and 2/ that could be memory expensive depending of the number of max connections.

- If we agree on the current proposal then I'll do  some refactoring in 
pg_stat_get_backend_io(), pg_stat_get_my_io() and pg_stat_get_io() to avoid
duplicated code (it's not done yet to ease the review).

Regards,

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

  9. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-09-09T06:29:18Z

    Hi,

On Fri, Sep 06, 2024 at 03:03:17PM +0000, Bertrand Drouvot wrote:
> - If we agree on the current proposal then I'll do  some refactoring in 
> pg_stat_get_backend_io(), pg_stat_get_my_io() and pg_stat_get_io() to avoid
> duplicated code (it's not done yet to ease the review).

Please find attached v3, mandatory rebase due to fc415edf8c.

Regards,

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

  10. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-09-13T04:25:46Z

    Hi,

On Fri, Sep 06, 2024 at 03:03:17PM +0000, Bertrand Drouvot wrote:
> The struct size is 1040 Bytes and that's much more reasonable than the size
> needed for per backend I/O stats in v1 (about 16KB).

One could think that's a high increase of shared memory usage with a high
number of connections (due to the fact that it's implemented as "fixed amount"
stats based on the max number of backends).

So out of curiosity, I did some measurements with:

dynamic_shared_memory_type=sysv
shared_memory_type=sysv
max_connections=20000

On my lab, ipcs -m gives me:

- on master

key        shmid      owner      perms      bytes      nattch     status
0x00113a04 51347487   postgres   600        1149394944 6
0x4bc9f2fa 51347488   postgres   600        4006976    6
0x46790800 51347489   postgres   600        1048576    2

- with v3 applied

key        shmid      owner      perms      bytes      nattch     status
0x00113a04 51347477   postgres   600        1170227200 6
0x08e04b0a 51347478   postgres   600        4006976    6
0x74688c9c 51347479   postgres   600        1048576    2

So, with 20000 max_connections (not advocating that's a reasonable number in 
all the cases), that's a 1170227200 - 1149394944 = 20 MB increase of shared
memory (expected with 20K max_connections and the new struct size of 1040 Bytes)
as compare to master which is 1096 MB.  It means that v3 produces about 2% shared
memory increase with 20000 max_connections.

Out of curiosity with max_connections=100000, then:

- on master:

------ Shared Memory Segments --------
key        shmid      owner      perms      bytes      nattch     status
0x00113a04 52953134   postgres   600        5053915136 6
0x37abf5ce 52953135   postgres   600        20006976   6
0x71c07d5c 52953136   postgres   600        1048576    2

- with v3:

------ Shared Memory Segments --------
key        shmid      owner      perms      bytes      nattch     status
0x00113a04 52953144   postgres   600        5157945344 6
0x7afb24de 52953145   postgres   600        20006976   6
0x2695af58 52953146   postgres   600        1048576    2

So, with 100000 max_connections (not advocating that's a reasonable number in 
all the cases), that's a 5157945344 - 5053915136 = 100 MB increase of shared
memory (expected with 100K max_connections and the new struct size of 1040 Bytes)
as compare to master which is about 4800 MB. It means that v3 produces about
2% shared memory increase with 100000 max_connections.


Then, based on those numbers, I think that the "fixed amount" approach is a good
one as 1/ the amount of shared memory increase is "relatively" small and 2/ most
probably provides performance benefits as compare to the "non fixed" approach.

Regards,

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

  11. Re: per backend I/O statistics

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2024-09-13T13:45:08Z

    Hi,

Thanks for working on this!

Your patch applies and builds cleanly.

On Fri, 6 Sept 2024 at 18:03, Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> - As stated up-thread, the pg_stat_get_backend_io() behaves as if
> stats_fetch_consistency is set to none (each execution re-fetches counters
> from shared memory). Indeed, the snapshot is not build in each backend to copy
> all the others backends stats, as 1/ there is no use case (there is no need to
> get others backends I/O statistics while taking care of the stats_fetch_consistency)
> and 2/ that could be memory expensive depending of the number of max connections.

I believe this is the correct approach.

I manually tested your patches, and they work as expected. Here is
some feedback:

- The stats_reset column is NULL in both pg_my_stat_io and
pg_stat_get_backend_io() until the first call to reset io statistics.
While I'm not sure if it's necessary, it might be useful to display
the more recent of the two times in the stats_reset column: the
statistics reset time or the backend creation time.

- The pgstat_reset_io_counter_internal() is called in the
pgstat_shutdown_hook(). This causes the stats_reset column showing the
termination time of the old backend when its proc num is reassigned to
a new backend.

--
Regards,
Nazir Bilal Yavuz
Microsoft

  12. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-09-13T16:09:40Z

    Hi,

On Fri, Sep 13, 2024 at 04:45:08PM +0300, Nazir Bilal Yavuz wrote:
> Hi,
> 
> Thanks for working on this!
> 
> Your patch applies and builds cleanly.

Thanks for looking at it!

> On Fri, 6 Sept 2024 at 18:03, Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> >
> > - As stated up-thread, the pg_stat_get_backend_io() behaves as if
> > stats_fetch_consistency is set to none (each execution re-fetches counters
> > from shared memory). Indeed, the snapshot is not build in each backend to copy
> > all the others backends stats, as 1/ there is no use case (there is no need to
> > get others backends I/O statistics while taking care of the stats_fetch_consistency)
> > and 2/ that could be memory expensive depending of the number of max connections.
> 
> I believe this is the correct approach.

Thanks for sharing your thoughts.

> I manually tested your patches, and they work as expected. Here is
> some feedback:
> 
> - The stats_reset column is NULL in both pg_my_stat_io and
> pg_stat_get_backend_io() until the first call to reset io statistics.
> While I'm not sure if it's necessary, it might be useful to display
> the more recent of the two times in the stats_reset column: the
> statistics reset time or the backend creation time.

I'm not sure about that as one can already get the backend "creation time"
through pg_stat_activity.backend_start.

> - The pgstat_reset_io_counter_internal() is called in the
> pgstat_shutdown_hook(). This causes the stats_reset column showing the
> termination time of the old backend when its proc num is reassigned to
> a new backend.

doh! Nice catch, thanks!

And also new backends that are not re-using a previous "existing" process slot
are getting the last reset time (if any). So the right place to fix this is in
pgstat_io_init_backend_cb(): done in v4 attached. v4 also sets the reset time to
0 in pgstat_shutdown_hook() (but that's not necessary though, that's just to be
right "conceptually" speaking).

Regards,

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

  13. Re: per backend I/O statistics

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2024-09-17T11:52:01Z

    Hi,

On Fri, 13 Sept 2024 at 19:09, Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
> On Fri, Sep 13, 2024 at 04:45:08PM +0300, Nazir Bilal Yavuz wrote:
> > - The pgstat_reset_io_counter_internal() is called in the
> > pgstat_shutdown_hook(). This causes the stats_reset column showing the
> > termination time of the old backend when its proc num is reassigned to
> > a new backend.
>
> doh! Nice catch, thanks!
>
> And also new backends that are not re-using a previous "existing" process slot
> are getting the last reset time (if any). So the right place to fix this is in
> pgstat_io_init_backend_cb(): done in v4 attached. v4 also sets the reset time to
> 0 in pgstat_shutdown_hook() (but that's not necessary though, that's just to be
> right "conceptually" speaking).

Thanks, I confirm that it is fixed.

You mentioned some refactoring upthread:

On Fri, 6 Sept 2024 at 18:03, Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> - If we agree on the current proposal then I'll do  some refactoring in
> pg_stat_get_backend_io(), pg_stat_get_my_io() and pg_stat_get_io() to avoid
> duplicated code (it's not done yet to ease the review).

Could we remove pg_stat_get_my_io() completely and use
pg_stat_get_backend_io() with the current backend's pid to get the
current backend's stats? If you meant the same thing, please don't
mind it.

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

  14. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-09-17T13:07:07Z

    Hi,

On Tue, Sep 17, 2024 at 02:52:01PM +0300, Nazir Bilal Yavuz wrote:
> Hi,
> 
> On Fri, 13 Sept 2024 at 19:09, Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> > On Fri, Sep 13, 2024 at 04:45:08PM +0300, Nazir Bilal Yavuz wrote:
> > > - The pgstat_reset_io_counter_internal() is called in the
> > > pgstat_shutdown_hook(). This causes the stats_reset column showing the
> > > termination time of the old backend when its proc num is reassigned to
> > > a new backend.
> >
> > doh! Nice catch, thanks!
> >
> > And also new backends that are not re-using a previous "existing" process slot
> > are getting the last reset time (if any). So the right place to fix this is in
> > pgstat_io_init_backend_cb(): done in v4 attached. v4 also sets the reset time to
> > 0 in pgstat_shutdown_hook() (but that's not necessary though, that's just to be
> > right "conceptually" speaking).
> 
> Thanks, I confirm that it is fixed.

Thanks!

> You mentioned some refactoring upthread:
> 
> On Fri, 6 Sept 2024 at 18:03, Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> >
> > - If we agree on the current proposal then I'll do  some refactoring in
> > pg_stat_get_backend_io(), pg_stat_get_my_io() and pg_stat_get_io() to avoid
> > duplicated code (it's not done yet to ease the review).
> 
> Could we remove pg_stat_get_my_io() completely and use
> pg_stat_get_backend_io() with the current backend's pid to get the
> current backend's stats?

The reason why I keep pg_stat_get_my_io() is because (as mentioned in [1]), the
statistics snapshot is build for "my backend stats" (means it depends of the
stats_fetch_consistency value). I can see use cases for that.

OTOH, pg_stat_get_backend_io() behaves as if stats_fetch_consistency is set to
none (each execution re-fetches counters from shared memory) (see [2]). Indeed,
the snapshot is not build in each backend to copy all the others backends stats,
as 1/ I think that there is no use case (there is no need to get others backends
I/O statistics while taking care of the stats_fetch_consistency) and 2/ that
could be memory expensive depending of the number of max connections.

So I think it's better to keep both functions as they behave differently.

Thoughts?

> If you meant the same thing, please don't
> mind it.

What I meant to say is to try to remove the duplicate code that we can find in
those 3 functions (say creating a new function that would contain the duplicate
code and make use of this new function in the 3 others). Did not look at it in
details yet.

[1]: https://www.postgresql.org/message-id/ZtXR%2BCtkEVVE/LHF%40ip-10-97-1-34.eu-west-3.compute.internal
[2]: https://www.postgresql.org/message-id/ZtsZtaRza9bFFeF8%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

  15. Re: per backend I/O statistics

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2024-09-17T13:47:51Z

    Hi,

On Tue, 17 Sept 2024 at 16:07, Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
> On Tue, Sep 17, 2024 at 02:52:01PM +0300, Nazir Bilal Yavuz wrote:
> > Could we remove pg_stat_get_my_io() completely and use
> > pg_stat_get_backend_io() with the current backend's pid to get the
> > current backend's stats?
>
> The reason why I keep pg_stat_get_my_io() is because (as mentioned in [1]), the
> statistics snapshot is build for "my backend stats" (means it depends of the
> stats_fetch_consistency value). I can see use cases for that.
>
> OTOH, pg_stat_get_backend_io() behaves as if stats_fetch_consistency is set to
> none (each execution re-fetches counters from shared memory) (see [2]). Indeed,
> the snapshot is not build in each backend to copy all the others backends stats,
> as 1/ I think that there is no use case (there is no need to get others backends
> I/O statistics while taking care of the stats_fetch_consistency) and 2/ that
> could be memory expensive depending of the number of max connections.
>
> So I think it's better to keep both functions as they behave differently.
>
> Thoughts?

Yes, that is correct. Sorry, you already had explained it and I had
agreed with it but I forgot.

> > If you meant the same thing, please don't
> > mind it.
>
> What I meant to say is to try to remove the duplicate code that we can find in
> those 3 functions (say creating a new function that would contain the duplicate
> code and make use of this new function in the 3 others). Did not look at it in
> details yet.

I got it, thanks for the explanation.

--
Regards,
Nazir Bilal Yavuz
Microsoft

  16. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-09-17T13:56:34Z

    Hi,

On Tue, Sep 17, 2024 at 04:47:51PM +0300, Nazir Bilal Yavuz wrote:
> Hi,
> 
> On Tue, 17 Sept 2024 at 16:07, Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> > So I think it's better to keep both functions as they behave differently.
> >
> > Thoughts?
> 
> Yes, that is correct. Sorry, you already had explained it and I had
> agreed with it but I forgot.

No problem at all! (I re-explained because I'm not always 100% sure that my
explanations are crystal clear ;-) )

Regards,

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

  17. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2024-09-20T03:53:43Z

    On Wed, Sep 04, 2024 at 04:45:24AM +0000, Bertrand Drouvot wrote:
> On Tue, Sep 03, 2024 at 04:07:58PM +0900, Kyotaro Horiguchi wrote:
>> As an additional benefit of this approach, the client can set a
>> connection variable, for example, no_backend_iostats to true, or set
>> its inverse variable to false, to restrict memory usage to only the
>> required backends.
> 
> Thanks for the feedback!
> 
> If we were to add an on/off switch button, I think I'd vote for a global one
> instead. Indeed, I see this feature more like an "Administrator" one, where
> the administrator wants to be able to find out which session is reponsible of
> what (from an I/O point of view): like being able to anwser "which session is
> generating this massive amount of reads"?
> 
> If we allow each session to disable the feature then the administrator
> would lost this ability.

Hmm, I've been studying this patch, and I am not completely sure to
agree with this feeling of using fixed-numbered stats, actually, after
reading the whole and seeing the structure of the patch
(FLEXIBLE_ARRAY_MEMBER is a new way to handle the fact that we don't
know exactly the number of slots we need to know for the
fixed-numbered stats as MaxBackends may change).  If we make these
kind of stats variable-numbered, does it have to actually involve many
creations or removals of the stats entries, though?  One point is that
the number of entries to know about is capped by max_connections,
which is a PGC_POSTMASTER.  That's the same kind of control as
replication slots.  So one approach would be to reuse entries in the
dshash and use in the hashing key the number in the procarrays.  If a
new connection spawns and reuses a slot that was used in the past,
then reset all the existing fields and assign its PID.

Another thing is the consistency of the data that we'd like to keep at
shutdown.  If the connections have a balanced amount of stats shared
among them, doing decision-making based on them is kind of easy.  But
that may cause confusion if the activity is unbalanced across the
sessions.  We could also not flush them to disk as an option, but it
still seems more useful to me to save this data across restarts if one
takes frequent snapshots of the new system view reporting everything,
so as it is possible to get an idea of the deltas across the snapshots
for each connection slot.
--
Michael

  18. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2024-09-20T04:26:49Z

    On Tue, Sep 17, 2024 at 01:56:34PM +0000, Bertrand Drouvot wrote:
> No problem at all! (I re-explained because I'm not always 100% sure that my
> explanations are crystal clear ;-) )

We've discussed a bit this patch offline, but after studying the patch
I doubt that this part is a good idea:

+	/* has to be at the end due to FLEXIBLE_ARRAY_MEMBER */
+	PgStatShared_IO io;
 } PgStat_ShmemControl;

We are going to be in trouble if we introduce a second member in this
routine that has a FLEXIBLE_ARRAY_MEMBER, because PgStat_ShmemControl
relies on the fact that all its members after deterministic offset
positions in this structure.  So this lacks flexibility.  This choice
is caused by the fact that we don't exactly know the number of
backends because that's controlled by the PGC_POSTMASTER GUC
max_connections so the size of the structure would be undefined.

There is a parallel with replication slot statistics here, where we
save the replication slot data in the dshash based on their index
number in shmem.  Hence, wouldn't it be better to do like replication
slot stats, where we use the dshash and a key based on the procnum of
each backend or auxiliary process (ProcNumber in procnumber.h)?  If at
restart max_connections is lower than what was previously used, we
could discard entries that would not fit anymore into the charts.
This is probably not something that happens often, so I'm not really
worried about forcing the removal of these stats depending on how the
upper-bound of ProcNumber evolves.

So, using a new kind of ID and making this kind variable-numbered may
ease the implementation quite a bit, while avoiding any extensibility
issues with the shmem portion of the patch if these are
fixed-numbered.  The reporting of these stats comes down to having a
parallel with pgstat_count_io_op_time(), but to make sure that the
stats are split by connection slot number rather than the current
split of pg_stat_io.  All its callers are in localbuf.c, bufmgr.c and
md.c, living with some duplication in the code paths to gather the
stats may be OK.

pg_stat_get_my_io() is based on a O(n^3).  IOOBJECT_NUM_TYPES is
fortunately low, still that's annoying.

This would rely on the fact that we would use the ProcNumber for the
dshash key, and this information is not provided in pg_stat_activity.
Perhaps we should add this information in pg_stat_activity so as it
would be easily possible to do joins with a SQL function that returns
a SRF with all the stats associated with a given connection slot
(auxiliary or backend process)?  That would be a separate patch.
Perhaps that's even something that has popped up for the work with
threading (did not follow this part closely, TBH)?

The active PIDs of the live sessions are not stored in the active
stats, why not?  Perhaps that's useless anyway if we expose the
ProcNumbers in pg_stat_activity and make the stats available with a
single function taking in input a ProcNumber.  Just mentioning an
option to consider.
--
Michael

  19. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-10-07T09:52:22Z

    Hi,

On Fri, Sep 20, 2024 at 12:53:43PM +0900, Michael Paquier wrote:
> On Wed, Sep 04, 2024 at 04:45:24AM +0000, Bertrand Drouvot wrote:
> > On Tue, Sep 03, 2024 at 04:07:58PM +0900, Kyotaro Horiguchi wrote:
> >> As an additional benefit of this approach, the client can set a
> >> connection variable, for example, no_backend_iostats to true, or set
> >> its inverse variable to false, to restrict memory usage to only the
> >> required backends.
> > 
> > Thanks for the feedback!
> > 
> > If we were to add an on/off switch button, I think I'd vote for a global one
> > instead. Indeed, I see this feature more like an "Administrator" one, where
> > the administrator wants to be able to find out which session is reponsible of
> > what (from an I/O point of view): like being able to anwser "which session is
> > generating this massive amount of reads"?
> > 
> > If we allow each session to disable the feature then the administrator
> > would lost this ability.
> 
> Hmm, I've been studying this patch,

Thanks for looking at it!

> and I am not completely sure to
> agree with this feeling of using fixed-numbered stats, actually, after
> reading the whole and seeing the structure of the patch
> (FLEXIBLE_ARRAY_MEMBER is a new way to handle the fact that we don't
> know exactly the number of slots we need to know for the
> fixed-numbered stats as MaxBackends may change).

Right, that's a new way of dealing with "unknown" number of slots (and it has
cons as you mentioned in [1]).

> If we make these
> kind of stats variable-numbered, does it have to actually involve many
> creations or removals of the stats entries, though?  One point is that
> the number of entries to know about is capped by max_connections,
> which is a PGC_POSTMASTER.  That's the same kind of control as
> replication slots.  So one approach would be to reuse entries in the
> dshash and use in the hashing key the number in the procarrays.  If a
> new connection spawns and reuses a slot that was used in the past,
> then reset all the existing fields and assign its PID.

Yeah, like it's done currently with the "fixed-numbered" stats proposal. That
sounds reasonable to me, I'll look at this proposed approach and come back with
a new patch version, thanks!

> Another thing is the consistency of the data that we'd like to keep at
> shutdown.  If the connections have a balanced amount of stats shared
> among them, doing decision-making based on them is kind of easy.  But
> that may cause confusion if the activity is unbalanced across the
> sessions.  We could also not flush them to disk as an option, but it
> still seems more useful to me to save this data across restarts if one
> takes frequent snapshots of the new system view reporting everything,
> so as it is possible to get an idea of the deltas across the snapshots
> for each connection slot.

The idea that has been implemented so far in this patch is that we still maintain
an aggregated version of the stats (visible through pg_stat_io) and that only the
aggregated stats are flushed/read to/from disk (means we don't flush the
per-backend stats).

I think that it makes sense that way. The way I see it is that the per-backend
I/O stats is more for current activity instrumentation. So it's not clear to me
what would be the benefits of restoring the per-backend stats at startup knowing
that: 1) we restored the aggregated stats and 2) the sessions that were responsible
for the the restored stats are gone.

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

Regards,

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

  20. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-10-07T09:54:21Z

    Hi,

On Fri, Sep 20, 2024 at 01:26:49PM +0900, Michael Paquier wrote:
> On Tue, Sep 17, 2024 at 01:56:34PM +0000, Bertrand Drouvot wrote:
> > No problem at all! (I re-explained because I'm not always 100% sure that my
> > explanations are crystal clear ;-) )
> 
> We've discussed a bit this patch offline, but after studying the patch
> I doubt that this part is a good idea:
> 
> +	/* has to be at the end due to FLEXIBLE_ARRAY_MEMBER */
> +	PgStatShared_IO io;
>  } PgStat_ShmemControl;
> 
> We are going to be in trouble if we introduce a second member in this
> routine that has a FLEXIBLE_ARRAY_MEMBER, because PgStat_ShmemControl
> relies on the fact that all its members after deterministic offset
> positions in this structure.

Agree that it would be an issue should we have to add a new FLEXIBLE_ARRAY_MEMBER.

> So this lacks flexibility.  This choice
> is caused by the fact that we don't exactly know the number of
> backends because that's controlled by the PGC_POSTMASTER GUC
> max_connections so the size of the structure would be undefined.

Right.

> There is a parallel with replication slot statistics here, where we
> save the replication slot data in the dshash based on their index
> number in shmem.  Hence, wouldn't it be better to do like replication
> slot stats, where we use the dshash and a key based on the procnum of
> each backend or auxiliary process (ProcNumber in procnumber.h)?  If at
> restart max_connections is lower than what was previously used, we
> could discard entries that would not fit anymore into the charts.
> This is probably not something that happens often, so I'm not really
> worried about forcing the removal of these stats depending on how the
> upper-bound of ProcNumber evolves.

Yeah, I'll look at implementing the dshash based on their procnum and see
where it goes.

> So, using a new kind of ID and making this kind variable-numbered may
> ease the implementation quite a bit, while avoiding any extensibility
> issues with the shmem portion of the patch if these are
> fixed-numbered.

Agree.

> This would rely on the fact that we would use the ProcNumber for the
> dshash key, and this information is not provided in pg_stat_activity.
> Perhaps we should add this information in pg_stat_activity so as it
> would be easily possible to do joins with a SQL function that returns
> a SRF with all the stats associated with a given connection slot
> (auxiliary or backend process)?

I'm not sure that's needed. What has been done in the previous versions is
to get the stats based on the pid (see pg_stat_get_backend_io()) where the
procnumber is retrieved with something like GetNumberFromPGProc(BackendPidGetProc(pid)).

Do you see an issue with that approach?

> The active PIDs of the live sessions are not stored in the active
> stats, why not? 

That was not needed. We can still retrieve the stats based on the pid thanks
to something like GetNumberFromPGProc(BackendPidGetProc(pid)) without having
to actually store the pid in the stats. I think that's fine because the pid
only matters at "display" time (pg_stat_get_backend_io()).

Regards,

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

  21. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2024-10-08T04:46:23Z

    On Mon, Oct 07, 2024 at 09:54:21AM +0000, Bertrand Drouvot wrote:
> On Fri, Sep 20, 2024 at 01:26:49PM +0900, Michael Paquier wrote:
>> This would rely on the fact that we would use the ProcNumber for the
>> dshash key, and this information is not provided in pg_stat_activity.
>> Perhaps we should add this information in pg_stat_activity so as it
>> would be easily possible to do joins with a SQL function that returns
>> a SRF with all the stats associated with a given connection slot
>> (auxiliary or backend process)?
> 
> I'm not sure that's needed. What has been done in the previous versions is
> to get the stats based on the pid (see pg_stat_get_backend_io()) where the
> procnumber is retrieved with something like GetNumberFromPGProc(BackendPidGetProc(pid)).

Ah, I see.  So you could just have the proc number in the key to
control the upper-bound on the number of possible stats entries in the
dshash.

Assuming that none of this data is persisted to the stats file at
shutdown and that the stats of a single entry are reset each time a
new backend reuses a previous proc slot, that would be OK by me, I
guess.

>> The active PIDs of the live sessions are not stored in the active
>> stats, why not? 
> 
> That was not needed. We can still retrieve the stats based on the pid thanks
> to something like GetNumberFromPGProc(BackendPidGetProc(pid)) without having
> to actually store the pid in the stats. I think that's fine because the pid
> only matters at "display" time (pg_stat_get_backend_io()).

Okay, per the above and the persistency of the stats.
--
Michael

  22. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-10-08T16:28:39Z

    Hi,

On Tue, Oct 08, 2024 at 01:46:23PM +0900, Michael Paquier wrote:
> On Mon, Oct 07, 2024 at 09:54:21AM +0000, Bertrand Drouvot wrote:
> > On Fri, Sep 20, 2024 at 01:26:49PM +0900, Michael Paquier wrote:
> >> This would rely on the fact that we would use the ProcNumber for the
> >> dshash key, and this information is not provided in pg_stat_activity.
> >> Perhaps we should add this information in pg_stat_activity so as it
> >> would be easily possible to do joins with a SQL function that returns
> >> a SRF with all the stats associated with a given connection slot
> >> (auxiliary or backend process)?
> > 
> > I'm not sure that's needed. What has been done in the previous versions is
> > to get the stats based on the pid (see pg_stat_get_backend_io()) where the
> > procnumber is retrieved with something like GetNumberFromPGProc(BackendPidGetProc(pid)).
> 
> Ah, I see.  So you could just have the proc number in the key to
> control the upper-bound on the number of possible stats entries in the
> dshash.

Yes.

> Assuming that none of this data is persisted to the stats file at
> shutdown and that the stats of a single entry are reset each time a
> new backend reuses a previous proc slot, that would be OK by me, I
> guess.
> 
> >> The active PIDs of the live sessions are not stored in the active
> >> stats, why not? 
> > 
> > That was not needed. We can still retrieve the stats based on the pid thanks
> > to something like GetNumberFromPGProc(BackendPidGetProc(pid)) without having
> > to actually store the pid in the stats. I think that's fine because the pid
> > only matters at "display" time (pg_stat_get_backend_io()).
> 
> Okay, per the above and the persistency of the stats.

Great, I'll work on an updated patch version then.

Regards,

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

  23. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-10-31T05:09:56Z

    Hi,

On Tue, Oct 08, 2024 at 04:28:39PM +0000, Bertrand Drouvot wrote:
> > > On Fri, Sep 20, 2024 at 01:26:49PM +0900, Michael Paquier wrote:
> > 
> > Okay, per the above and the persistency of the stats.
> 
> Great, I'll work on an updated patch version then.
> 

I spend some time on this during the last 2 days and I think we have 3 design
options.

=== GOALS ===

But first let's sump up the goals that I think we agreed on:

- Keep pg_stat_io as it is today: give the whole server picture and serialize
the stats to disk.

- Introduce per-backend IO stats and 2 new APIs to:

   1. Provide the IO stats for "my backend" (through say pg_my_stat_io), this
      would take care of the stats_fetch_consistency.

   2. Retrieve the IO stats for another backend (through say pg_stat_get_backend_io(pid))
      that would _not_ take care of stats_fetch_consistency, as:

      2.1/ I think that there is no use case (there is no need to get others
           backends I/O statistics while taking care of the stats_fetch_consistency)

      2.2/ That could be memory expensive to store a snapshot for all the backends
		   (depending of the number of backend created)

- There is no need to serialize the per-backend IO stats to disk (no point to
see stats for backends that do not exist anymore after a re-start).

- The per-backend IO stats should be variable-numbered (not fixed), as per 
up-thread discussion.

=== OPTIONS ===

So, based on this, I think that we could:

Option 1: "move" the existing PGSTAT_KIND_IO to variable-numbered and let this
KIND take care of the aggregated view (pg_stat_io) and the per-backend stats.

Option 2: let PGSTAT_KIND_IO as it is and introduce a new PGSTAT_KIND_BACKEND_IO
that would be variable-numbered.

Option 3: Remove PGSTAT_KIND_IO, introduce a new PGSTAT_KIND_BACKEND_IO that
would be variable-numbered and store the "aggregated stats aka pg_stat_io" in
shared memory (not part of the variable-numbered hash). Per-backend stats
could be aggregated into "pg_stat_io" during the flush_pending_cb call for example.

=== BEST OPTION? ===

I would opt for Option 2 as:

- The stats system is currently not designed for Option 1 and our goals (for
example the shared_data_len is used to serialize but also to fetch the entries,
see pgstat_fetch_entry()) so that would need some hack to serialize only a part
of them and still be able to fetch them all).

- Mixing "fixed" and "variable" in the same KIND does not sound like a good idea
(though that might be possible with some hacks, I don't think that would be 
easy to maintain).

- Having the per-backend as "variable" in its dedicated kind looks more reasonable
and less error-prone.

- I don't think there is a stats design similar to option 3 currently, so I'm
not sure there is a need to develop something new while Option 2 could be done.

- Option 3 would need some hack for (at least) the "pg_stat_io" [de]serialization
part.

- Option 2 seems to offer more flexibility (as compare to Option 1 and 3).

Thoughts?

Regards,

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

  24. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-11-04T10:01:50Z

    Hi,

On Thu, Oct 31, 2024 at 05:09:56AM +0000, Bertrand Drouvot wrote:
> === OPTIONS ===
> 
> So, based on this, I think that we could:
> 
> Option 1: "move" the existing PGSTAT_KIND_IO to variable-numbered and let this
> KIND take care of the aggregated view (pg_stat_io) and the per-backend stats.
> 
> Option 2: let PGSTAT_KIND_IO as it is and introduce a new PGSTAT_KIND_BACKEND_IO
> that would be variable-numbered.
> 
> Option 3: Remove PGSTAT_KIND_IO, introduce a new PGSTAT_KIND_BACKEND_IO that
> would be variable-numbered and store the "aggregated stats aka pg_stat_io" in
> shared memory (not part of the variable-numbered hash). Per-backend stats
> could be aggregated into "pg_stat_io" during the flush_pending_cb call for example.
> 
> === BEST OPTION? ===
> 
> I would opt for Option 2 as:
> 
> - The stats system is currently not designed for Option 1 and our goals (for
> example the shared_data_len is used to serialize but also to fetch the entries,
> see pgstat_fetch_entry()) so that would need some hack to serialize only a part
> of them and still be able to fetch them all).
> 
> - Mixing "fixed" and "variable" in the same KIND does not sound like a good idea
> (though that might be possible with some hacks, I don't think that would be 
> easy to maintain).
> 
> - Having the per-backend as "variable" in its dedicated kind looks more reasonable
> and less error-prone.
> 
> - I don't think there is a stats design similar to option 3 currently, so I'm
> not sure there is a need to develop something new while Option 2 could be done.
> 
> - Option 3 would need some hack for (at least) the "pg_stat_io" [de]serialization
> part.
> 
> - Option 2 seems to offer more flexibility (as compare to Option 1 and 3).
> 
> Thoughts?

And why not add more per-backend stats in the future? (once the I/O part is done).

I think that's one more reason to go with option 2 (and implementing a brand new
PGSTAT_KIND_BACKEND kind).

Thoughts?

Regards,

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

  25. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-11-05T17:37:15Z

    Hi,

On Mon, Nov 04, 2024 at 10:01:50AM +0000, Bertrand Drouvot wrote:
> And why not add more per-backend stats in the future? (once the I/O part is done).
> 
> I think that's one more reason to go with option 2 (and implementing a brand new
> PGSTAT_KIND_BACKEND kind).

I'm starting working on option 2, I think it will be easier to discuss with
a patch proposal to look at.

If in the meantime, one strongly disagree with option 2 (means implement a brand
new PGSTAT_KIND_BACKEND and keep PGSTAT_KIND_IO), please let me know.

Regards,

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

  26. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2024-11-05T23:39:07Z

    On Tue, Nov 05, 2024 at 05:37:15PM +0000, Bertrand Drouvot wrote:
> I'm starting working on option 2, I think it will be easier to discuss with
> a patch proposal to look at.
> 
> If in the meantime, one strongly disagree with option 2 (means implement a brand
> new PGSTAT_KIND_BACKEND and keep PGSTAT_KIND_IO), please let me know.

Sorry for the late reply, catching up a bit.

As you are quoting in [1], you do not expect the backend-io stats and
the more global pg_stat_io to achieve the same level of consistency as
the backend stats would be gone at restart, and wiped out when a
backend shuts down.  So, splitting them with a different stats kind
feels more natural because it would be possible to control how each
stat kind behaves depending on the code shutdown and reset paths
within their own callbacks rather than making the callbacks of
PGSTAT_KIND_IO more complex than they already are.  And pg_stat_io is
a fixed-numbered stats kind because of the way it aggregates its stats
with a number states defined at compile-time.

Is the structure you have in mind different than PgStat_BktypeIO?
Perhaps a split is better anyway with that in mind.

The amount of memory required to store the snapshots of backend-IO
does not worry me much, TBH, but you are worried about a high turnover
of connections that could cause a lot of bloat in the backend-IO
snapshots because of the persistency that these stats would have,
right?  Actually, we already have cases with other stats kinds where
it is possible for a backend to hold references to some stats on an
object that has been dropped concurrently.  At least, that's possible
when a backend shuts down.  If possible, supporting snapshots would be
more consistent with the other stats.

Just to be clear, I am not in favor of making PgStat_HashKey larger
than it already is.  With 8 bytes allocated for the object ID, the
chances of conflicts in the dshash for a single variable-numbered
stats kind play in our favor already even with a couple of million
entries.

[1]: https://www.postgresql.org/message-id/ZyMRJIbUpNPoCXUe@ip-10-97-1-34.eu-west-3.compute.internal
--
Michael

  27. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-11-06T13:51:02Z

    Hi,

On Wed, Nov 06, 2024 at 08:39:07AM +0900, Michael Paquier wrote:
> On Tue, Nov 05, 2024 at 05:37:15PM +0000, Bertrand Drouvot wrote:
> > I'm starting working on option 2, I think it will be easier to discuss with
> > a patch proposal to look at.
> > 
> > If in the meantime, one strongly disagree with option 2 (means implement a brand
> > new PGSTAT_KIND_BACKEND and keep PGSTAT_KIND_IO), please let me know.
> 
> Sorry for the late reply, catching up a bit.

No problem at all, thanks for looking at it!

> As you are quoting in [1], you do not expect the backend-io stats and
> the more global pg_stat_io to achieve the same level of consistency as
> the backend stats would be gone at restart, and wiped out when a
> backend shuts down.

Yes.

> So, splitting them with a different stats kind
> feels more natural because it would be possible to control how each
> stat kind behaves depending on the code shutdown and reset paths
> within their own callbacks rather than making the callbacks of
> PGSTAT_KIND_IO more complex than they already are.

Yeah, thanks for sharing your thoughts.

> And pg_stat_io is
> a fixed-numbered stats kind because of the way it aggregates its stats
> with a number states defined at compile-time.
> 
> Is the structure you have in mind different than PgStat_BktypeIO?

Very close.

> Perhaps a split is better anyway with that in mind.

The in-progress patch (not shared yet) is using the following:

"
typedef struct PgStat_Backend
{
       TimestampTz stat_reset_timestamp;
       BackendType bktype;
       PgStat_BktypeIO stats;
} PgStat_Backend;
"

The bktype is used to be able to filter the stats correctly when we display them.

> The amount of memory required to store the snapshots of backend-IO
> does not worry me much, TBH, but you are worried about a high turnover
> of connections that could cause a lot of bloat in the backend-IO
> snapshots because of the persistency that these stats would have,
> right?

Not only a high turnover but also a high number of entries created in the hash.

Furthermore I don't see any use case of relying on stats_fetch_consistency
while querying other backend's stats.

> If possible, supporting snapshots would be
> more consistent with the other stats.

I have I mind to support the snapshots _only_ when querying our own stats. I can
measure the memory impact if we use them also when querying other backends stats
too (though I don't see a use case).

> Just to be clear, I am not in favor of making PgStat_HashKey larger
> than it already is.

That's not needed, the patch I'm working on stores the proc number in the
objid field of the key.

Regards,

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

  28. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2024-11-07T00:50:59Z

    On Wed, Nov 06, 2024 at 01:51:02PM +0000, Bertrand Drouvot wrote:
> That's not needed, the patch I'm working on stores the proc number in the
> objid field of the key.

Relying on the procnumber for the object ID gets a +1 here.  That
provides an automatic cap on the maximum number of entries that can
exist at once for this new stats kind.
--
Michael

  29. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-11-07T16:32:44Z

    Hi,

On Thu, Nov 07, 2024 at 09:50:59AM +0900, Michael Paquier wrote:
> On Wed, Nov 06, 2024 at 01:51:02PM +0000, Bertrand Drouvot wrote:
> > That's not needed, the patch I'm working on stores the proc number in the
> > objid field of the key.
> 
> Relying on the procnumber for the object ID gets a +1 here.

Thanks!

> That
> provides an automatic cap on the maximum number of entries that can
> exist at once for this new stats kind.

Yeah.

POC patch is now working, will wear a reviewer hat now and will share in one 
or 2 days.

Regards,

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

  30. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-11-08T14:09:30Z

    Hi,

On Thu, Nov 07, 2024 at 04:32:44PM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Thu, Nov 07, 2024 at 09:50:59AM +0900, Michael Paquier wrote:
> > On Wed, Nov 06, 2024 at 01:51:02PM +0000, Bertrand Drouvot wrote:
> > > That's not needed, the patch I'm working on stores the proc number in the
> > > objid field of the key.
> > 
> > Relying on the procnumber for the object ID gets a +1 here.
> 
> Thanks!
> 
> > That
> > provides an automatic cap on the maximum number of entries that can
> > exist at once for this new stats kind.
> 
> Yeah.
> 
> POC patch is now working, will wear a reviewer hat now and will share in one 
> or 2 days.
> 

Please find attached v5 that implements the per-backend IO stats as 
variable-numbered stats kind (as per the up-thread discussion).

It is split into 4 sub-patches:

==== 0001 (the largest one)

Introduces a new statistics KIND. It is named PGSTAT_KIND_PER_BACKEND as it could
be used in the future to store other statistics (than the I/O ones) per backend.
The new KIND is a variable-numbered one and has an automatic cap on the
maximum number of entries (as its hash key contains the proc number).

There is no need to serialize the per backend I/O stats to disk (no point to
see stats for backends that do not exist anymore after a re-start), so a
new "to_serialize" field is added in the PgStat_KindInfo struct.

It adds a new pg_my_stat_io view to display "my" backend I/O statistics.

It also adds a new function pg_stat_reset_single_backend_io_counters() to be
able to reset the I/O stats for a given backend pid.

It contains doc updates and dedicated tests.

A few remarks:

1. one assertion in pgstat_drop_entry_internal() is not necessary true anymore 
with this new stat kind. So, adding an extra bool as parameter to take care of it.

2. a new struct "PgStat_Backend" is created and does contain the backend type.
The backend type is used for filtering purpose when the stats are displayed.

3. when the stats are reset, we need to keep the backend type so the change
in shared_stat_reset_contents().

4. the pending stats are updated in the existing pgstat_count_io_op_n() and 
pgstat_count_io_op_time() functions.

5. this new kind has its own flush callback: pgstat_per_backend_flush_cb().
But there is no need to maintain 2 callbacks for the I/O stats, so 0002 will
merge both and remove the one from the fixed stats. The reason it's not done
in 0001 is to ease the review.

==== 0002

Merge both IO stats flush callback. There is no need to keep both callbacks.

Merging both allows to save O(N^3) while looping on IOOBJECT_NUM_TYPES,
IOCONTEXT_NUM_TYPES and IOCONTEXT_NUM_TYPES.

The patch removes:

1. pgstat_io_flush_cb()
2. PendingIOStats (as the same information is already stored in the pending
entries)
3. have_iostats (the patch relies on the pending entries instead)

==== 0003

Don't include other backend's stats in the snapshot.

When stats_fetch_consistency is set to 'snapshot', don't include other backend's
stats in the snapshot. There is no use case, so save memory usage.

==== 0004

Adding the pg_stat_get_backend_io() function to retrieve I/O statistics for
a particular backend pid.

Note that this function does not return any rows if stats_fetch_consistency
is set to 'snapshot' and the pid of interest is not our own pid (there is no use
case of retrieving other backend's stats with stats_fetch_consistency set to
'snapshot').

The patch:

1. replaces pg_stat_get_my_io() with pg_stat_get_backend_io()
2. adds pgstat_fetch_proc_stat_io()
3. adds doc
4. adds tests

=== Remarks

R1. The key field is still "objid" even if it stores the proc number. I think
that's fine and there is no need to rename it as it's related comment states:

"
 /* object ID (table, function, etc.), or identifier. */
"

R2. pg_stat_get_io() and pg_stat_get_backend_io() could probably be merged (
duplicated code). That's not mandatory to provide the new per backend I/O stats
feature. So let's keep it as future work to ease the review.

R3. There is no use case of retrieving other backend's IO stats with
stats_fetch_consistency set to 'snapshot'. Avoiding this behavior allows us to
save memory (that could be non negligeable as pgstat_build_snapshot() would add
_all_ the other backend's stats in the snapshot). I choose to document it and to
not return any rows: I think that's better than returning some data (that would
not "ensure" the snapshot consistency) or an error.

Looking forward to your feedback,

Regards,

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

  31. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2024-11-14T06:31:51Z

    On Fri, Nov 08, 2024 at 02:09:30PM +0000, Bertrand Drouvot wrote:
> ==== 0001 (the largest one)
> 
> Introduces a new statistics KIND. It is named PGSTAT_KIND_PER_BACKEND as it could
> be used in the future to store other statistics (than the I/O ones) per backend.
> The new KIND is a variable-numbered one and has an automatic cap on the
> maximum number of entries (as its hash key contains the proc number).
> 
> There is no need to serialize the per backend I/O stats to disk (no point to
> see stats for backends that do not exist anymore after a re-start), so a
> new "to_serialize" field is added in the PgStat_KindInfo struct.
> 
> It adds a new pg_my_stat_io view to display "my" backend I/O statistics.
> 
> It also adds a new function pg_stat_reset_single_backend_io_counters() to be
> able to reset the I/O stats for a given backend pid.
> 
> It contains doc updates and dedicated tests.
> 
> A few remarks:
> 
> 1. one assertion in pgstat_drop_entry_internal() is not necessary true anymore 
> with this new stat kind. So, adding an extra bool as parameter to take care of it.

Why?  I have to admit that the addition of this argument at this level
specific to a new stats kind feels really weird and it looks like a
layer violation, while this happens only when resetting the whole
pgstats state because we have loaded a portion of the stats but the
file loaded was in such a state that we don't have a consistent
picture.

> 2. a new struct "PgStat_Backend" is created and does contain the backend type.
> The backend type is used for filtering purpose when the stats are displayed.

Is that necessary?  We can guess that from the PID with a join in
pg_stat_activity.  pg_stat_get_backend_io() from 0004 returns a set of
tuples for a specific PID, as well..

+	/* save the bktype */
+	if (kind == PGSTAT_KIND_PER_BACKEND)
+		bktype = ((PgStatShared_Backend *) header)->stats.bktype;
[...]
+	/* restore the bktype */
+	if (kind == PGSTAT_KIND_PER_BACKEND)
+		((PgStatShared_Backend *) header)->stats.bktype = bktype;

Including the backend type results in these blips in
shared_stat_reset_contents() which should not have anything related 
to stats kinds and should remain neutral, as well.

pgstat_prep_per_backend_pending() is used only in pgstat_io.c, so I
guess that it should be static in this file rather than declared in
pgstat.h?

+typedef struct PgStat_PendingIO

Perhaps this part should use a separate structure named
"BackendPendingIO"?  The definition of the structure has to be in
pgstat.h as this is the pending_size of the new stats kind.  It looks
like it would be cleaner to keep PgStat_PendingIO local to
pgstat_io.c, and define PgStat_PendingIO based on
PgStat_BackendPendingIO?

+	/*
+	 * Do serialize or not this kind of stats.
+	 */
+	bool		to_serialize:1;

Not sure that "serialize" is the best term that applies here.  For
pgstats entries, serialization refers to the matter of writing their
entries with a "serialized" name because they have an undefined number
when stored locally after a reload.  I'd suggest to split this concept
into its own patch, rename the flag as "write_to_file" (or what you
think is suited), and also apply the flag in the fixed-numbered loop
done in pgstat_write_statsfile() before going through the dshash.

+      <row>
+       <entry role="func_table_entry"><para role="func_signature">
+        <indexterm>
+         <primary>pg_stat_reset_single_backend_io_counters</primary>
+        </indexterm>
+        <function>pg_stat_reset_single_backend_io_counters</function> ( <type>int4</type> )
+        <returnvalue>void</returnvalue>

This should document that the input argument is a PID.

Is pg_my_stat_io() the best name ever?  I'd suggest to just merge 0004
with 0001.

Structurally, it may be cleaner to keep all the callbacks and the
backend-I/O specific logic into a separate file, perhaps
pgstat_io_backend.c or pgstat_backend_io?

> ==== 0002
> 
> Merge both IO stats flush callback. There is no need to keep both callbacks.
> 
> Merging both allows to save O(N^3) while looping on IOOBJECT_NUM_TYPES,
> IOCONTEXT_NUM_TYPES and IOCONTEXT_NUM_TYPES.

Not sure to be a fan of that, TBH, still I get the performance
argument of the matter.  Each stats kind has its own flush path, and
this assumes that we'll never going to diverge in terms of the
information maintained for each backend and the existing pg_stat_io.
Perhaps there could be a divergence at some point?

> === Remarks
> 
> R1. The key field is still "objid" even if it stores the proc number. I think
> that's fine and there is no need to rename it as it's related comment states:

Let's not change the object ID and the hash key.  The approach you are
using here with a variable-numbered stats kinds and a lookup with the
procnumber is sensible here.

> R2. pg_stat_get_io() and pg_stat_get_backend_io() could probably be merged (
> duplicated code). That's not mandatory to provide the new per backend I/O stats
> feature. So let's keep it as future work to ease the review.

Not sure about that.

> R3. There is no use case of retrieving other backend's IO stats with
> stats_fetch_consistency set to 'snapshot'. Avoiding this behavior allows us to
> save memory (that could be non negligeable as pgstat_build_snapshot() would add
> _all_ the other backend's stats in the snapshot). I choose to document it and to
> not return any rows: I think that's better than returning some data (that would
> not "ensure" the snapshot consistency) or an error.

I'm pretty sure that there is a sensible argument about being able to
get a snapshot of the I/O stat of another backend and consider that a
feature.  For example, keeping a look at the activity of the
checkpointer while doing a scan at a certain point in time?  With a
GUC, we could have both behaviors and control which one we want.
--
Michael

  32. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-11-14T13:30:11Z

    Hi,

On Thu, Nov 14, 2024 at 03:31:51PM +0900, Michael Paquier wrote:
> On Fri, Nov 08, 2024 at 02:09:30PM +0000, Bertrand Drouvot wrote:
> > 1. one assertion in pgstat_drop_entry_internal() is not necessary true anymore 
> > with this new stat kind. So, adding an extra bool as parameter to take care of it.
> 
> Why?  I have to admit that the addition of this argument at this level
> specific to a new stats kind feels really weird and it looks like a
> layer violation, while this happens only when resetting the whole
> pgstats state because we have loaded a portion of the stats but the
> file loaded was in such a state that we don't have a consistent
> picture.

Yes. I agree that looks weird and I don't like it that much. But the assertion
is not true anymore. If you:

- change the arguments to false in the pgstat_drop_entry_internal() call in 
pgstat_drop_all_entries()
- start the engine
- kill -9 postgres
- restart the engine

You'll see the assert failing due to the startup process. I don't think it is
doing something wrong though, just populating its backend stats. Do you have
another view on it?

> > 2. a new struct "PgStat_Backend" is created and does contain the backend type.
> > The backend type is used for filtering purpose when the stats are displayed.
> 
> Is that necessary?  We can guess that from the PID with a join in
> pg_stat_activity.  pg_stat_get_backend_io() from 0004 returns a set of
> tuples for a specific PID, as well..

How would that work for someone doing "select * from pg_stat_get_backend_io(<pid>)"?

Do you mean copy/paste part of the code from pg_stat_get_activity() into
pg_stat_get_backend_io() to get the backend type? That sounds like an idea,
I'll have a look at it.

> +	/* save the bktype */
> +	if (kind == PGSTAT_KIND_PER_BACKEND)
> +		bktype = ((PgStatShared_Backend *) header)->stats.bktype;
> [...]
> +	/* restore the bktype */
> +	if (kind == PGSTAT_KIND_PER_BACKEND)
> +		((PgStatShared_Backend *) header)->stats.bktype = bktype;
> 
> Including the backend type results in these blips in
> shared_stat_reset_contents() which should not have anything related 
> to stats kinds and should remain neutral, as well.

Yeah, we can simply get rid of it if we remove the backend type in PgStat_Backend.

> pgstat_prep_per_backend_pending() is used only in pgstat_io.c, so I
> guess that it should be static in this file rather than declared in
> pgstat.h?

Good catch, thanks!

> +typedef struct PgStat_PendingIO
> 
> Perhaps this part should use a separate structure named
> "BackendPendingIO"?  The definition of the structure has to be in
> pgstat.h as this is the pending_size of the new stats kind.  It looks
> like it would be cleaner to keep PgStat_PendingIO local to
> pgstat_io.c, and define PgStat_PendingIO based on
> PgStat_BackendPendingIO?

I see what you meean, what about simply "PgStat_BackendPending" in pgstat.h?

> +	/*
> +	 * Do serialize or not this kind of stats.
> +	 */
> +	bool		to_serialize:1;
> 
> Not sure that "serialize" is the best term that applies here.  For
> pgstats entries, serialization refers to the matter of writing their
> entries with a "serialized" name because they have an undefined number
> when stored locally after a reload.  I'd suggest to split this concept
> into its own patch, rename the flag as "write_to_file" (or what you
> think is suited), and also apply the flag in the fixed-numbered loop
> done in pgstat_write_statsfile() before going through the dshash.

Makes sense to create its own patch, will have a look.

> +      <row>
> +       <entry role="func_table_entry"><para role="func_signature">
> +        <indexterm>
> +         <primary>pg_stat_reset_single_backend_io_counters</primary>
> +        </indexterm>
> +        <function>pg_stat_reset_single_backend_io_counters</function> ( <type>int4</type> )
> +        <returnvalue>void</returnvalue>
> 
> This should document that the input argument is a PID.

Yeap, will add.

> 
> Is pg_my_stat_io() the best name ever? 

Not 100% sure, but there is already "pg_my_temp_schema" so I thought that
pg_my_stat_io would not be that bad. Open to suggestions though.

> I'd suggest to just merge 0004 with 0001.

Sure.

> Structurally, it may be cleaner to keep all the callbacks and the
> backend-I/O specific logic into a separate file, perhaps
> pgstat_io_backend.c or pgstat_backend_io?

Yeah, I was wondering the same. What about pgstat_backend.c (that would contain
only one "section" dedicated to IO currently)?

> > ==== 0002
> > 
> > Merge both IO stats flush callback. There is no need to keep both callbacks.
> > 
> > Merging both allows to save O(N^3) while looping on IOOBJECT_NUM_TYPES,
> > IOCONTEXT_NUM_TYPES and IOCONTEXT_NUM_TYPES.
> 
> Not sure to be a fan of that, TBH, still I get the performance
> argument of the matter.  Each stats kind has its own flush path, and
> this assumes that we'll never going to diverge in terms of the
> information maintained for each backend and the existing pg_stat_io.
> Perhaps there could be a divergence at some point?

Yeah perhaps, so in case of divergence let's split "again"? I mean for the moment
I don't see any reason to keep both and we have pros related to efficiency during
flush.

> > === Remarks
> > 
> > R1. The key field is still "objid" even if it stores the proc number. I think
> > that's fine and there is no need to rename it as it's related comment states:
> 
> Let's not change the object ID and the hash key.  The approach you are
> using here with a variable-numbered stats kinds and a lookup with the
> procnumber is sensible here.

Agree, thanks for confirming.

> > R2. pg_stat_get_io() and pg_stat_get_backend_io() could probably be merged (
> > duplicated code). That's not mandatory to provide the new per backend I/O stats
> > feature. So let's keep it as future work to ease the review.
> 
> Not sure about that.

I don't have a strong opinion about this one, so I'm ok to not merge those.

> > R3. There is no use case of retrieving other backend's IO stats with
> > stats_fetch_consistency set to 'snapshot'. Avoiding this behavior allows us to
> > save memory (that could be non negligeable as pgstat_build_snapshot() would add
> > _all_ the other backend's stats in the snapshot). I choose to document it and to
> > not return any rows: I think that's better than returning some data (that would
> > not "ensure" the snapshot consistency) or an error.
> 
> I'm pretty sure that there is a sensible argument about being able to
> get a snapshot of the I/O stat of another backend and consider that a
> feature.  For example, keeping a look at the activity of the
> checkpointer while doing a scan at a certain point in time?

hm, not sure how the stats_fetch_consistency set to 'snapshot' could help here:

- the checkpointer could write stuff for reason completly unrelated to "your"
backend

- if the idea is to 1. record checkpointer stats, 2. do stuff in the backend and
3. check again the checkpointer stats then I don't think setting stats_fetch_consistency
to snapshot is needed at all. In fact it's quite the opposite as 1. and 3. would
report the same values with stats_fetch_consistency set to 'snapshot' (if done 
in the same transaction).

Regards,

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

  33. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2024-11-19T01:47:53Z

    On Thu, Nov 14, 2024 at 01:30:11PM +0000, Bertrand Drouvot wrote:
> - change the arguments to false in the pgstat_drop_entry_internal() call in 
> pgstat_drop_all_entries()
> - start the engine
> - kill -9 postgres
> - restart the engine
> 
> You'll see the assert failing due to the startup process. I don't think it is
> doing something wrong though, just populating its backend stats. Do you have
> another view on it?

It feels sad that we have to plug in the internals at this level for
this particular case.  Perhaps there is something to do with more
callbacks.  Or perhaps there is just no point in tracking the stats of
auxiliary processes because we already have this data in the existing
pg_stat_io already?

> How would that work for someone doing "select * from pg_stat_get_backend_io(<pid>)"?
> 
> Do you mean copy/paste part of the code from pg_stat_get_activity() into
> pg_stat_get_backend_io() to get the backend type? That sounds like an idea,
> I'll have a look at it.

I was under the impression that we should keep PgStat_Backend for the
reset_timestamp part, but drop BackendType as it can be guessed from
pg_stat_activity itself.  For example a LATERAL to grab the current
live stats of these backends.

>> +typedef struct PgStat_PendingIO
>> 
>> Perhaps this part should use a separate structure named
>> "BackendPendingIO"?  The definition of the structure has to be in
>> pgstat.h as this is the pending_size of the new stats kind.  It looks
>> like it would be cleaner to keep PgStat_PendingIO local to
>> pgstat_io.c, and define PgStat_PendingIO based on
>> PgStat_BackendPendingIO?
> 
> I see what you meean, what about simply "PgStat_BackendPending" in pgstat.h?

That should be OK.

>> Structurally, it may be cleaner to keep all the callbacks and the
>> backend-I/O specific logic into a separate file, perhaps
>> pgstat_io_backend.c or pgstat_backend_io?
> 
> Yeah, I was wondering the same. What about pgstat_backend.c (that would contain
> only one "section" dedicated to IO currently)?

pgstat_backend.c looks good to me.  Could there be other stats than
just IO, actually?  Perhaps not, just asking..

> - if the idea is to 1. record checkpointer stats, 2. do stuff in the backend and
> 3. check again the checkpointer stats then I don't think setting stats_fetch_consistency
> to snapshot is needed at all. In fact it's quite the opposite as 1. and 3. would
> report the same values with stats_fetch_consistency set to 'snapshot' (if done 
> in the same transaction).

Hmm.  Not sure.  It looks like you're right to treat this in a
separate patch as it would exist for different reasons than the
original proposal.
--
Michael

  34. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-11-19T16:28:55Z

    Hi,

On Tue, Nov 19, 2024 at 10:47:53AM +0900, Michael Paquier wrote:
> On Thu, Nov 14, 2024 at 01:30:11PM +0000, Bertrand Drouvot wrote:
> > - change the arguments to false in the pgstat_drop_entry_internal() call in 
> > pgstat_drop_all_entries()
> > - start the engine
> > - kill -9 postgres
> > - restart the engine
> > 
> > You'll see the assert failing due to the startup process. I don't think it is
> > doing something wrong though, just populating its backend stats. Do you have
> > another view on it?
> 
> It feels sad that we have to plug in the internals at this level for
> this particular case.  Perhaps there is something to do with more
> callbacks.  Or perhaps there is just no point in tracking the stats of
> auxiliary processes because we already have this data in the existing
> pg_stat_io already?

We can not discard the "per backend" stats collection for auxiliary processes
because those are the source for pg_stat_io too (once the 2 flush callbacks are
merged).

I have another idea: after a bit more of investigation it turns out that
only the startup process is generating the failed assertion.

So, for the startup process only, what about?

- don't call pgstat_create_backend_stat() in pgstat_beinit()...
- but call it in StartupXLOG() instead (after the stats are discarded or restored).

That way we could get rid of the changes linked to the assertion and still handle
the startup process particular case. Thoughts?

> > How would that work for someone doing "select * from pg_stat_get_backend_io(<pid>)"?
> > 
> > Do you mean copy/paste part of the code from pg_stat_get_activity() into
> > pg_stat_get_backend_io() to get the backend type? That sounds like an idea,
> > I'll have a look at it.
> 
> I was under the impression that we should keep PgStat_Backend for the
> reset_timestamp part, but drop BackendType as it can be guessed from
> pg_stat_activity itself.

Removing BackendType sounds doable, I'll look at it.

> >> Structurally, it may be cleaner to keep all the callbacks and the
> >> backend-I/O specific logic into a separate file, perhaps
> >> pgstat_io_backend.c or pgstat_backend_io?
> > 
> > Yeah, I was wondering the same. What about pgstat_backend.c (that would contain
> > only one "section" dedicated to IO currently)?
> 
> pgstat_backend.c looks good to me.  Could there be other stats than
> just IO, actually?  Perhaps not, just asking..

Yeah that's the reason why I suggested "pgstat_backend.c". I would not be
surprised if we add more "per backend" stats (that are not I/O related) in the
future as the main machinery would be there.

Regards,

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

  35. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2024-11-20T00:01:26Z

    On Tue, Nov 19, 2024 at 04:28:55PM +0000, Bertrand Drouvot wrote:
> So, for the startup process only, what about?
> 
> - don't call pgstat_create_backend_stat() in pgstat_beinit()...
> - but call it in StartupXLOG() instead (after the stats are discarded or restored).
> 
> That way we could get rid of the changes linked to the assertion and still handle
> the startup process particular case. Thoughts?

Hmm.  That may prove to be a good idea in the long-term.  The startup
process is a specific path kicked in at a very early stage, so it is
also a bit weird that we'd try to insert statistics while we are going
to reset them anyway a bit later.  That may also be relevant for
custom statistics, actually, especially if some calls happen in some
hook paths taken before the reset is done.  This could happen for
injection point stats when loading it with shared_preload_libraries, 
actually, if you load, attach or run a callback at this early stage.
The existing sanity checks are a safety net that we should not change
or adapt for specific stats kinds conditions, IMO.  Perhaps this had
better be done as a patch of its own, on top of the rest.

> Yeah that's the reason why I suggested "pgstat_backend.c". I would not be
> surprised if we add more "per backend" stats (that are not I/O related) in the
> future as the main machinery would be there.

Okay.
--
Michael

  36. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-11-20T14:10:23Z

    Hi,

On Thu, Nov 14, 2024 at 03:31:51PM +0900, Michael Paquier wrote:
> +	/*
> +	 * Do serialize or not this kind of stats.
> +	 */
> +	bool		to_serialize:1;
> 
> Not sure that "serialize" is the best term that applies here.  For
> pgstats entries, serialization refers to the matter of writing their
> entries with a "serialized" name because they have an undefined number
> when stored locally after a reload.  I'd suggest to split this concept
> into its own patch, rename the flag as "write_to_file" 

Yeah, also this could useful for custom statistics. So I created a dedicated
thread and a patch proposal (see [1]).

[1]: https://www.postgresql.org/message-id/Zz3skBqzBncSFIuY%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

  37. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-11-20T14:20:18Z

    Hi,

On Wed, Nov 20, 2024 at 09:01:26AM +0900, Michael Paquier wrote:
> On Tue, Nov 19, 2024 at 04:28:55PM +0000, Bertrand Drouvot wrote:
> > So, for the startup process only, what about?
> > 
> > - don't call pgstat_create_backend_stat() in pgstat_beinit()...
> > - but call it in StartupXLOG() instead (after the stats are discarded or restored).
> > 
> > That way we could get rid of the changes linked to the assertion and still handle
> > the startup process particular case. Thoughts?
> 
> Hmm.  That may prove to be a good idea in the long-term.  The startup
> process is a specific path kicked in at a very early stage, so it is
> also a bit weird that we'd try to insert statistics while we are going
> to reset them anyway a bit later.

Exactly.

> That may also be relevant for
> custom statistics, actually, especially if some calls happen in some
> hook paths taken before the reset is done.  This could happen for
> injection point stats when loading it with shared_preload_libraries, 
> actually, if you load, attach or run a callback at this early stage.

Right. I did not had in mind to go that far here (for the per backend stats
needs). My idea was "just" to move the new pgstat_create_backend_stat() (which
is related to per backend stats only) call at the right place in StartupXLOG()
for the startup process only. As that's the startup process that will reset
or restore the stats I think that makes sense.

It looks like that what you have in mind is much more generic, what about:

- Focus on this thread first and then move the call as proposed above
- Think about a more generic idea later on (on the per-backend I/O stats is
in).

Thoughts?

Regards,

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

  38. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2024-11-20T22:18:24Z

    On Wed, Nov 20, 2024 at 02:20:18PM +0000, Bertrand Drouvot wrote:
> Right. I did not had in mind to go that far here (for the per backend stats
> needs). My idea was "just" to move the new pgstat_create_backend_stat() (which
> is related to per backend stats only) call at the right place in StartupXLOG()
> for the startup process only. As that's the startup process that will reset
> or restore the stats I think that makes sense.
> 
> It looks like that what you have in mind is much more generic, what about:
> 
> - Focus on this thread first and then move the call as proposed above
> - Think about a more generic idea later on (on the per-backend I/O stats is
> in).

Moving pgstat_create_backend_stat() may be OK in the long-term, at
least that would document why we need to care about the startup
process.

Still, moving only the call feels incomplete, so how about adding a
boolean field in PgStat_ShmemControl that defaults to false when the
shmem area is initialized in StatsShmemInit(), then switched to true
once we know that the stats have been restored or reset by the startup
process.  Something like that should work to control the dshash
inserts, I guess?
--
Michael

  39. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2024-11-21T00:42:03Z

    On Wed, Nov 20, 2024 at 02:10:23PM +0000, Bertrand Drouvot wrote:
> Yeah, also this could useful for custom statistics. So I created a dedicated
> thread and a patch proposal (see [1]).
> 
> [1]: https://www.postgresql.org/message-id/Zz3skBqzBncSFIuY%40ip-10-97-1-34.eu-west-3.compute.internal

Thanks for putting that on its own thread.  Will look at it.
--
Michael

  40. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-11-21T17:23:42Z

    Hi,

On Thu, Nov 21, 2024 at 07:18:24AM +0900, Michael Paquier wrote:
> On Wed, Nov 20, 2024 at 02:20:18PM +0000, Bertrand Drouvot wrote:
> > Right. I did not had in mind to go that far here (for the per backend stats
> > needs). My idea was "just" to move the new pgstat_create_backend_stat() (which
> > is related to per backend stats only) call at the right place in StartupXLOG()
> > for the startup process only. As that's the startup process that will reset
> > or restore the stats I think that makes sense.
> > 
> > It looks like that what you have in mind is much more generic, what about:
> > 
> > - Focus on this thread first and then move the call as proposed above
> > - Think about a more generic idea later on (on the per-backend I/O stats is
> > in).
> 
> Moving pgstat_create_backend_stat() may be OK in the long-term, at
> least that would document why we need to care about the startup
> process.

Yeap.

> Still, moving only the call feels incomplete, so how about adding a
> boolean field in PgStat_ShmemControl that defaults to false when the
> shmem area is initialized in StatsShmemInit(), then switched to true
> once we know that the stats have been restored or reset by the startup
> process.  Something like that should work to control the dshash
> inserts, I guess?

I did some tests (on a primary and on a standby) and currently there is no call
to pgstat_get_entry_ref() at all before we reach the stats reset or restore
in StartupXLOG(). The first ones appear after "we really open for business"
(quoting process_pm_child_exit()).

Now, with the per-backend I/O stats patch in place, there is 3 processes that
could call pgstat_get_entry_ref() before we reach the stats reset or restore in
StartupXLOG() (observed by setting a breakpoint on the startup process before
stats are reset or restored):

- checkpointer
- background writer
- startup process

All calls are for the new backend stat kind. It also makes sense to see non
"regular" backends as the "regulars" are not allowed to connect yet.

Results: they could collect some stats for nothing since a reset or restore will
happen after.

Now, if we want to prevent new entries to be created before the stats are
reset or restored:

- then the end results will be the same (means starting the instance with reset
or restored stats). The only difference would be that they would not collect stats
for "nothing" during this small time window.

- it looks like that would lead to some non negligible refactoring: I started the
coding and observed the potential impact. The reason is that all the code (at least
the one I looked at) does not expect to receive a NULL value from pgstat_get_entry_ref()
(when asking for creation) and we would need to take care of all the cases
(all the stats, all backend type) for safety reason.

So, given that:

- the end result would be the same
- the code changes would be non negligible (unless we have a better idea than
pgstat_get_entry_ref() returning a NULL value).

I think that might be valid to try to implement it (to avoid those processes to
collect the stats for nothing during this small time window) but I am not sure
that's worth it (as the end result would be the same and the code change non
negligible).

Thoughts?

Regards,

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

  41. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2024-11-22T01:36:29Z

    On Thu, Nov 21, 2024 at 05:23:42PM +0000, Bertrand Drouvot wrote:
> So, given that:
> 
> - the end result would be the same
> - the code changes would be non negligible (unless we have a better idea than
> pgstat_get_entry_ref() returning a NULL value).

Hmm.  created_entry only matters for pgstat_init_function_usage().
All the other callers of pgstat_prep_pending_entry() pass a NULL
value.  This makes me wonder if there is an argument for reworking a
bit that.  There's a fat comment about the reason why, still, that
would make the get interface for variable-numbered stats a lot
leaner..  Not sure what to think about that.
--
Michael

  42. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-11-22T07:49:58Z

    Hi,

On Fri, Nov 22, 2024 at 10:36:29AM +0900, Michael Paquier wrote:
> On Thu, Nov 21, 2024 at 05:23:42PM +0000, Bertrand Drouvot wrote:
> > So, given that:
> > 
> > - the end result would be the same
> > - the code changes would be non negligible (unless we have a better idea than
> > pgstat_get_entry_ref() returning a NULL value).
> 
> Hmm.  created_entry only matters for pgstat_init_function_usage().
> All the other callers of pgstat_prep_pending_entry() pass a NULL
> value. 

I meant to say all the calls that passe "create" as true in pgstat_get_entry_ref().

Regards,

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

  43. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2024-11-25T01:06:44Z

    On Fri, Nov 22, 2024 at 07:49:58AM +0000, Bertrand Drouvot wrote:
> On Fri, Nov 22, 2024 at 10:36:29AM +0900, Michael Paquier wrote:
>> Hmm.  created_entry only matters for pgstat_init_function_usage().
>> All the other callers of pgstat_prep_pending_entry() pass a NULL
>> value. 
> 
> I meant to say all the calls that passe "create" as true in pgstat_get_entry_ref().

Ah, OK, I think that I see your point here.

I am wondering how much this would matter as well for custom stats,
but we're not there yet without at least one release out and folks try
new things with these APIs and variable-numbered kinds.  Allowing
pgstat_prep_pending_entry() to return NULL even if "create" is true
may be a good thing, at the end, because that's the only way I can see
based on the current APIs where we could say "Sorry, but the stats
have not been loaded yet, so you cannot try to do anything related to
the dshash".

From my view having a kind of barrier would be cleaner in the long
run, but it's true that it may not be mandatory, as well.  pg_stat_io
is currently OK to be called because the stats are loaded for
auxiliary processes because it uses fixed-numbered stats in shmem.
And it means we already have early calls that add stats getting
overwritten once the stats are loaded from the on-disk file (Am I
getting this part right?).

Anyway, do we really require that for the sake of this thread?  We
know that there's only one of each auxiliary process at a time, and
they keep a footprint in pg_stat_io already.  So we could just limit
outselves to live database backends, WAL senders and autovacuum
workers, everything that's not auxiliary and spawned on request?
--
Michael

  44. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-11-25T07:12:56Z

    Hi,

On Mon, Nov 25, 2024 at 10:06:44AM +0900, Michael Paquier wrote:
> On Fri, Nov 22, 2024 at 07:49:58AM +0000, Bertrand Drouvot wrote:
> > On Fri, Nov 22, 2024 at 10:36:29AM +0900, Michael Paquier wrote:
> >> Hmm.  created_entry only matters for pgstat_init_function_usage().
> >> All the other callers of pgstat_prep_pending_entry() pass a NULL
> >> value. 
> > 
> > I meant to say all the calls that passe "create" as true in pgstat_get_entry_ref().
> 
> Ah, OK, I think that I see your point here.
> 
> I am wondering how much this would matter as well for custom stats,
> but we're not there yet without at least one release out and folks try
> new things with these APIs and variable-numbered kinds.

Not sure here, could custom stats start incrementing before the database system
is ready to accept connections?

> pgstat_prep_pending_entry() to return NULL even if "create" is true
> may be a good thing, at the end, because that's the only way I can see
> based on the current APIs where we could say "Sorry, but the stats
> have not been loaded yet, so you cannot try to do anything related to
> the dshash".

Yeah, same here.

> From my view having a kind of barrier would be cleaner in the long
> run, but it's true that it may not be mandatory, as well.  pg_stat_io
> is currently OK to be called because the stats are loaded for
> auxiliary processes because it uses fixed-numbered stats in shmem.
> And it means we already have early calls that add stats getting
> overwritten once the stats are loaded from the on-disk file (Am I
> getting this part right?).

Yeah, we can already see that, for example, the background writer could enter 
pgstat_io_flush_cb() before the stats are reset or restored.

> Anyway, do we really require that for the sake of this thread?  We
> know that there's only one of each auxiliary process at a time, and
> they keep a footprint in pg_stat_io already.  So we could just limit
> outselves to live database backends, WAL senders and autovacuum
> workers, everything that's not auxiliary and spawned on request?

I think that's a fair starting point and that we will not lose any informations
doing so (as you said there is only one of each auxiliary process at a time,
so that one could already see their stats from pg_stat_io). 

The only cons that I can see is that we will not be able to merge the flush cb
but I don't think that's a blocker (the flush are done in shared memory so the
impact on performance should not be that much of an issue).

I'll come back with a new version implementing the above.

[1]: https://www.postgresql.org/message-id/Zz9sno%2BJJbWqdXhQ%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

  45. Re: per backend I/O statistics[

    Michael Paquier <michael@paquier.xyz> — 2024-11-25T07:18:54Z

    On Mon, Nov 25, 2024 at 07:12:56AM +0000, Bertrand Drouvot wrote:
> Not sure here, could custom stats start incrementing before the database system
> is ready to accept connections?

In theory, that could be possible.  Like pg_stat_io currently, I am
ready to assume that it likely won't matter much.

> The only cons that I can see is that we will not be able to merge the flush cb
> but I don't think that's a blocker (the flush are done in shared memory so the
> impact on performance should not be that much of an issue).

The backend and I/O stats could begin diverging as a result of a new
implementation detail, and the performance of the flushes don't worry
me knowing at which frequency they happen on a live system.
--
Michael

  46. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-11-25T15:47:59Z

    Hi,

On Mon, Nov 25, 2024 at 04:18:54PM +0900, Michael Paquier wrote:
> On Mon, Nov 25, 2024 at 07:12:56AM +0000, Bertrand Drouvot wrote:
> > Not sure here, could custom stats start incrementing before the database system
> > is ready to accept connections?
> 
> In theory, that could be possible.  Like pg_stat_io currently, I am
> ready to assume that it likely won't matter much.

Yeah right, agree.

> > The only cons that I can see is that we will not be able to merge the flush cb
> > but I don't think that's a blocker (the flush are done in shared memory so the
> > impact on performance should not be that much of an issue).
> 
> The backend and I/O stats could begin diverging as a result of a new
> implementation detail, and the performance of the flushes don't worry
> me knowing at which frequency they happen on a live system.

Same here.

Please find attached v6 that enables per-backend I/O stats for the
B_AUTOVAC_WORKER, B_BACKEND, B_BG_WORKER, B_STANDALONE_BACKEND, B_SLOTSYNC_WORKER
and B_WAL_SENDER backend types.

It also takes care of most of the comments that you have made in [1], meaning
that it:

- removes the backend type from PgStat_Backend and look for the backend type
at "display" time.
- creates PgStat_BackendPendingIO and PgStat_PendingIO now refers to it (I 
used PgStat_BackendPendingIO and not PgStat_BackendPending because this is what
it is after all).
- adds the missing comment related to the PID in the doc.
- merges 0004 with 0001 (so that pg_stat_get_backend_io() is now part of 0001).
- creates its own pgstat_backend.c file.

=== Remarks

R1: as compared to v5, v6 removes the per-backend I/O stats reset from 
pg_stat_reset_shared(). I think it makes more sense that way, since we are
adding pg_stat_reset_single_backend_io_counters(). The per-backend I/O stats
behaves then as the subscription stats as far the reset is concerned.
 
R2: as we can't merge the flush cb anymore, only the patches related to
the stats_fetch_consistency/'snapshot' are missing in v6 (as compared to v5).
I propose to re-submit them, re-start the discussion once 0001 goes in.

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

Regards,

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

  47. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2024-11-27T06:33:38Z

    On Mon, Nov 25, 2024 at 03:47:59PM +0000, Bertrand Drouvot wrote:
> It also takes care of most of the comments that you have made in [1], meaning
> that it:
> 
> - removes the backend type from PgStat_Backend and look for the backend type
> at "display" time.
> - creates PgStat_BackendPendingIO and PgStat_PendingIO now refers to it (I 
> used PgStat_BackendPendingIO and not PgStat_BackendPending because this is what
> it is after all).
> - adds the missing comment related to the PID in the doc.
> - merges 0004 with 0001 (so that pg_stat_get_backend_io() is now part of 0001).
> - creates its own pgstat_backend.c file.

I have begun studying the patch, and I have one question.

+void
+pgstat_create_backend_stat(ProcNumber procnum)
[...]
+   /* Create the per-backend statistics entry */
+   if (pgstat_tracks_per_backend_bktype(MyBackendType))
+       pgstat_create_backend_stat(MyProcNumber);

Perhaps that's a very stupid question, but I was looking at this part
of the patch, and wondered why we don't use init_backend_cb?  I
vaguely recalled that MyProcNumber and MyBackendType would be set
before we go through the pgstat initialization step.  MyDatabaseId is
filled after the pgstat initialization, but we don't care about this
information for such backend stats.
--
Michael

  48. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-11-27T16:00:14Z

    Hi,

On Wed, Nov 27, 2024 at 03:33:38PM +0900, Michael Paquier wrote:
> On Mon, Nov 25, 2024 at 03:47:59PM +0000, Bertrand Drouvot wrote:
> > It also takes care of most of the comments that you have made in [1], meaning
> > that it:
> > 
> > - removes the backend type from PgStat_Backend and look for the backend type
> > at "display" time.
> > - creates PgStat_BackendPendingIO and PgStat_PendingIO now refers to it (I 
> > used PgStat_BackendPendingIO and not PgStat_BackendPending because this is what
> > it is after all).
> > - adds the missing comment related to the PID in the doc.
> > - merges 0004 with 0001 (so that pg_stat_get_backend_io() is now part of 0001).
> > - creates its own pgstat_backend.c file.
> 
> I have begun studying the patch, and I have one question.
> 
> +void
> +pgstat_create_backend_stat(ProcNumber procnum)
> [...]
> +   /* Create the per-backend statistics entry */
> +   if (pgstat_tracks_per_backend_bktype(MyBackendType))
> +       pgstat_create_backend_stat(MyProcNumber);
> 
> Perhaps that's a very stupid question, but I was looking at this part
> of the patch, and wondered why we don't use init_backend_cb?  I
> vaguely recalled that MyProcNumber and MyBackendType would be set
> before we go through the pgstat initialization step.  MyDatabaseId is
> filled after the pgstat initialization, but we don't care about this
> information for such backend stats.

Using init_backend_cb (and moving the pgstat_tracks_per_backend_bktype() check
in it) is currently producing a failed assertion:

TRAP: failed Assert("pgstat_is_initialized && !pgstat_is_shutdown"), File: "pgstat.c", Line: 1567, PID: 2080000
postgres: postgres postgres [local] initializing(ExceptionalCondition+0xbb)[0x5fd0c69afaed]
postgres: postgres postgres [local] initializing(pgstat_assert_is_up+0x3f)[0x5fd0c67d1b77]
postgres: postgres postgres [local] initializing(pgstat_get_entry_ref+0x95)[0x5fd0c67db068]
postgres: postgres postgres [local] initializing(pgstat_prep_pending_entry+0xaa)[0x5fd0c67d1181]
postgres: postgres postgres [local] initializing(pgstat_create_backend_stat+0x96)[0x5fd0c67d3986]

But even if we, say move the "pgstat_is_initialized = true" before the init_backend_cb()
call in pgstat_initialize() (not saying that makes sense, just trying out), then
we are back to the restore/reset issue but this time during initdb:

TRAP: failed Assert("!pgstat_entry_ref_hash_lookup(pgStatEntryRefHash, shent->key)"), File: "pgstat_shmem.c", Line: 852, PID: 2109086
/home/postgres/postgresql/pg_installed/pg18/bin/postgres(ExceptionalCondition+0xbb)[0x621723499c5a]
/home/postgres/postgresql/pg_installed/pg18/bin/postgres(+0x70dd2b)[0x6217232c5d2b]
/home/postgres/postgresql/pg_installed/pg18/bin/postgres(pgstat_drop_all_entries+0x61)[0x6217232c60b5]
/home/postgres/postgresql/pg_installed/pg18/bin/postgres(+0x705356)[0x6217232bd356]
/home/postgres/postgresql/pg_installed/pg18/bin/postgres(+0x70462a)[0x6217232bc62a]
/home/postgres/postgresql/pg_installed/pg18/bin/postgres(pgstat_restore_stats+0x1c)[0x6217232b9f01]
/home/postgres/postgresql/pg_installed/pg18/bin/postgres(StartupXLOG+0x693)[0x621722da9697]
/home/postgres/postgresql/pg_installed/pg18/bin/postgres(InitPostgres+0x1d8)[0x6217234b40df]
/home/postgres/postgresql/pg_installed/pg18/bin/postgres(BootstrapModeMain+0x532)[0x621722dd79bf]

where "PID: 2109086" is a B_STANDALONE_BACKEND.

This is due to the fact that, during the initdb bootstrap, init_backend_cb() is
called before StartupXLOG().

One option could be to move B_STANDALONE_BACKEND in the "false" section of 
pgstat_tracks_per_backend_bktype() and ensure that we set pgstat_is_initialized
to true before init_backend_cb() is called (but I don't think that makes that much 
sense).

I'd vote to just keep the pgstat_create_backend_stat() call in pgstat_beinit().

That said, we probably should document that init_backend_cb() should not call
pgstat_get_entry_ref(), but that's probably worth another thread.

Regards,

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

  49. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2024-12-12T04:11:09Z

    On Wed, Nov 27, 2024 at 04:00:14PM +0000, Bertrand Drouvot wrote:
> I'd vote to just keep the pgstat_create_backend_stat() call in pgstat_beinit().
> 
> That said, we probably should document that init_backend_cb() should not call
> pgstat_get_entry_ref(), but that's probably worth another thread.

Hmm, yeah.  I have considered this point and your approach does not
seem that bad to me after thinking much more about it, and you are
putting the call in pgstat_beinit(), which is about initializing the
stats for the backend.  So I'm OK with what the patch does here in
terms of the location where pgstat_create_backend_stat() is called.
--
Michael

  50. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2024-12-12T04:52:03Z

    On Mon, Nov 25, 2024 at 03:47:59PM +0000, Bertrand Drouvot wrote:
> === Remarks
> 
> R1: as compared to v5, v6 removes the per-backend I/O stats reset from 
> pg_stat_reset_shared(). I think it makes more sense that way, since we are
> adding pg_stat_reset_single_backend_io_counters(). The per-backend I/O stats
> behaves then as the subscription stats as far the reset is concerned.

Makes sense to me to not include that in pg_stat_reset_shared().

> R2: as we can't merge the flush cb anymore, only the patches related to
> the stats_fetch_consistency/'snapshot' are missing in v6 (as compared to v5).
> I propose to re-submit them, re-start the discussion once 0001 goes in.

Yeah, thanks.  I should think more about this part, but I'm still kind
of unconvinced.  Let's do things step by step.  For now, I have looked
at v6.

+        view. The function does not return I/O statistics for the checkpointer,
+        the background writer, the startup process and the autovacuum launcher
+        as they are already visible in the <link linkend="monitoring-pg-stat-io-view"> <structname>pg_stat_io</structname></link>
+        view and there is only one of those.

This last sentence seems unnecessary?  The function is named
"backend", and well, all these processes are not backends.

+    /*
+     * Maybe an auxiliary process? That should not be possible, due to
+     * pgstat_tracks_per_backend_bktype() though.
+     */
+    if (proc == NULL)
+        proc = AuxiliaryPidGetProc(backend_pid);
[...]
+        /*
+         * Maybe an auxiliary process? That should not be possible, due to
+         * pgstat_tracks_per_backend_bktype() though.
+         */
+        if (proc == NULL)
+            proc = AuxiliaryPidGetProc(pid);

This does not seem right.  Shouldn't we return immediately if
BackendPidGetProc() finds nothing matching with the PID?

+	/* Look for the backend type */
+	for (curr_backend = 1; curr_backend <= num_backends; curr_backend++)
+	{
+		LocalPgBackendStatus *local_beentry;
+		PgBackendStatus *beentry;
+
+		/* Get the next one in the list */
+		local_beentry = pgstat_get_local_beentry_by_index(curr_backend);
+		beentry = &local_beentry->backendStatus;
+
+		/* looking for specific PID, ignore all the others */
+		if (beentry->st_procpid != pid)
+			continue;
+
+		bktype = beentry->st_backendType;
+		break;
+	}

Sounds to me that the backend type is not strictly required in this
function call if pg_stat_activity can tell already that?

+       (void) pgstat_per_backend_flush_cb(entry_ref, nowait);

I'd recommend to not directly call the callback, use a wrapper
function instead if need be.

 pgstat_count_io_op_time(IOObject io_object, IOContext io_context, IOOp io_op,
 						instr_time start_time, uint32 cnt)
 {
+
 	if (track_io_timing)

Noise diff.

 /*
- * Simpler wrapper of pgstat_io_flush_cb()
+ * Simpler wrapper of pgstat_io_flush_cb() and pgstat_per_backend_flush_cb().
  */
 void
 pgstat_flush_io(bool nowait)

This is also called in the checkpointer and the bgwriter and the
walwriter via pgstat_report_wal(), which is kind of useless.  Perhaps
just use a different, separate function instead and use that where it
makes sense (per se also the argument of upthread that backend stats
may not be only IO-related..).

Sounds to me that PgStat_BackendPendingIO should be
PgStat_BackendPendingStats?

+{ oid => '8806', descr => 'statistics: per backend IO statistics',
+  proname => 'pg_stat_get_backend_io', prorows => '5', proretset => 't',

Similarly, s/pg_stat_get_backend_io/pg_stat_get_backend_stats/?

+  descr => 'statistics: reset collected IO statistics for a single backend',
+  proname => 'pg_stat_reset_single_backend_io_counters', provolatile => 'v', 

And here, pg_stat_reset_backend_stats?

 #define PGSTAT_KIND_SUBSCRIPTION   5   /* per-subscription statistics */
+#define PGSTAT_KIND_PER_BACKEND    6 

Missing one comment here.

FWIW, I'm so-so about the addition of pg_my_stat_io, knowing that
pg_stat_get_backend_io(NULL/pg_backend_pid()) does the same job.  I
would just add a note in the docs with a query showing how to use it
with pg_stat_activity.  An example with LATERAL, doing the same work:
select a.pid, s.* from pg_stat_activity as a,
   lateral pg_stat_get_backend_io(a.pid) as s
  where pid = pg_backend_pid();
--
Michael

  51. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-12-12T14:02:38Z

    Hi,

On Thu, Dec 12, 2024 at 01:52:03PM +0900, Michael Paquier wrote:
> On Mon, Nov 25, 2024 at 03:47:59PM +0000, Bertrand Drouvot wrote:

> +        view. The function does not return I/O statistics for the checkpointer,
> +        the background writer, the startup process and the autovacuum launcher
> +        as they are already visible in the <link linkend="monitoring-pg-stat-io-view"> <structname>pg_stat_io</structname></link>
> +        view and there is only one of those.
> 
> This last sentence seems unnecessary?  The function is named
> "backend", and well, all these processes are not backends.

Yeah, but you can find their pid through pg_stat_activity for which the pid field
description is "Process ID of this backend". Also we can find "backend_type" in
both pg_stat_activity and pg_stat_io, so I think this last sentence could help 
to avoid any confusion though.

> +    /*
> +     * Maybe an auxiliary process? That should not be possible, due to
> +     * pgstat_tracks_per_backend_bktype() though.
> +     */
> +    if (proc == NULL)
> +        proc = AuxiliaryPidGetProc(backend_pid);
> [...]
> +        /*
> +         * Maybe an auxiliary process? That should not be possible, due to
> +         * pgstat_tracks_per_backend_bktype() though.
> +         */
> +        if (proc == NULL)
> +            proc = AuxiliaryPidGetProc(pid);
> 
> This does not seem right.  Shouldn't we return immediately if
> BackendPidGetProc() finds nothing matching with the PID?

Yeah, that would work. I was keeping the AuxiliaryPidGetProc() calls just in case
we want to add the Aux processes back in the future. Replaced with a comment
in v7 attached instead.

> 
> +	/* Look for the backend type */
> +	for (curr_backend = 1; curr_backend <= num_backends; curr_backend++)
> +	{
> +		LocalPgBackendStatus *local_beentry;
> +		PgBackendStatus *beentry;
> +
> +		/* Get the next one in the list */
> +		local_beentry = pgstat_get_local_beentry_by_index(curr_backend);
> +		beentry = &local_beentry->backendStatus;
> +
> +		/* looking for specific PID, ignore all the others */
> +		if (beentry->st_procpid != pid)
> +			continue;
> +
> +		bktype = beentry->st_backendType;
> +		break;
> +	}
> 
> Sounds to me that the backend type is not strictly required in this
> function call if pg_stat_activity can tell already that?

Yeah, but it's needed in pg_stat_get_backend_io() for the stats filtering (to
display only those linked to this backend type), later in the function here:

+    /*
+     * Some combinations of BackendType, IOObject, and IOContext are
+     * not valid for any type of IOOp. In such cases, omit the entire
+     * row from the view.
+     */
+    if (!pgstat_tracks_io_object(bktype, io_obj, io_context))
+          continue;

OTOH, now that we get rid of the AuxiliaryPidGetProc() call in
pg_stat_reset_single_backend_io_counters() we can also remove the need to
look for the backend type in this function.

> 
> +       (void) pgstat_per_backend_flush_cb(entry_ref, nowait);
> 
> I'd recommend to not directly call the callback, use a wrapper
> function instead if need be.

Makes sense, created its own callback in pgstat_backend.c. Bonus point, it avoids
unnecessary pgstat_tracks_per_backend_bktype() checks in :

pgstat_report_bgwriter()
pgstat_report_checkpointer()
pgstat_report_wal()

as there is no attempt to call the callback anymore in those places.

>  /*
> - * Simpler wrapper of pgstat_io_flush_cb()
> + * Simpler wrapper of pgstat_io_flush_cb() and pgstat_per_backend_flush_cb().
>   */
>  void
>  pgstat_flush_io(bool nowait)
> 
> This is also called in the checkpointer and the bgwriter and the
> walwriter via pgstat_report_wal(), which is kind of useless.  Perhaps
> just use a different, separate function instead and use that where it
> makes sense (per se also the argument of upthread that backend stats
> may not be only IO-related..).

Yeah, done that way. 

BTW, not related to this particular patch but I realized that pgstat_flush_io()
is called for the walwriter. Indeed, it's coming from the pgstat_report_wal()
call in WalWriterMain(). That can not report any I/O stats activity (as the
walwriter is not part of the I/O stats tracking, see pgstat_tracks_io_bktype()).

So it looks like, we could move pgstat_flush_io() outside of pgstat_report_wal()
and add the pgstat_flush_io() calls only where they need to be made (and so, not
in WalWriterMain()).

Maybe a dedicated thread is worth it for that, thoughts?

> Sounds to me that PgStat_BackendPendingIO should be
> PgStat_BackendPendingStats?

Yeah, can do that as that's what is being used for the pending_size for
example.

OTOH, it's clear that this one in pgstat_io.c has to be "linked" to an "IO" 
related one:

"
typedef PgStat_BackendPendingStats PgStat_PendingIO;
"

The right way would probably be to do something like:

"
typedef struct PgStat_BackendPendingStats {
    PgStat_BackendPendingIO pendingio;  
} PgStat_BackendPendingStats;
"

But I'm not sure that's worth it until we don't have a need to add more
pending stats per-backend, thoughts?

> +{ oid => '8806', descr => 'statistics: per backend IO statistics',
> +  proname => 'pg_stat_get_backend_io', prorows => '5', proretset => 't',
> 
> Similarly, s/pg_stat_get_backend_io/pg_stat_get_backend_stats/?

I think that's fine to keep pg_stat_get_backend_io(). If we add more per-backend
stats in the future then we could add "dedicated" get functions too and a generic
one retrieving all of them.

> +  descr => 'statistics: reset collected IO statistics for a single backend',
> +  proname => 'pg_stat_reset_single_backend_io_counters', provolatile => 'v', 
> 
> And here, pg_stat_reset_backend_stats?

Same as above, we could imagine that in the future the backend would get mutiple
stats and that one would want to reset only the I/O ones for example.

>  #define PGSTAT_KIND_SUBSCRIPTION   5   /* per-subscription statistics */
> +#define PGSTAT_KIND_PER_BACKEND    6 
> 
> Missing one comment here.

Yeap.

> FWIW, I'm so-so about the addition of pg_my_stat_io, knowing that
> pg_stat_get_backend_io(NULL/pg_backend_pid()) does the same job.

Okay, I don't have a strong opinion about that. Removed in v7.

> I
> would just add a note in the docs with a query showing how to use it
> with pg_stat_activity.  An example with LATERAL, doing the same work:
> select a.pid, s.* from pg_stat_activity as a,
>    lateral pg_stat_get_backend_io(a.pid) as s
>   where pid = pg_backend_pid();

I'm not sure it's worth it. I think that's clear that to get our own stats
then we need to provide our own backend pid. For example pg_stat_get_activity()
does not provide such an example using pg_stat_activity or using something like 
pg_stat_get_activity(pg_backend_pid()).

Regards,

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

  52. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2024-12-13T02:02:53Z

    On Thu, Dec 12, 2024 at 02:02:38PM +0000, Bertrand Drouvot wrote:
> On Thu, Dec 12, 2024 at 01:52:03PM +0900, Michael Paquier wrote:
> Yeah, but it's needed in pg_stat_get_backend_io() for the stats filtering (to
> display only those linked to this backend type), later in the function here:
> 
> +    /*
> +     * Some combinations of BackendType, IOObject, and IOContext are
> +     * not valid for any type of IOOp. In such cases, omit the entire
> +     * row from the view.
> +     */
> +    if (!pgstat_tracks_io_object(bktype, io_obj, io_context))
> +          continue;
> 
> OTOH, now that we get rid of the AuxiliaryPidGetProc() call in
> pg_stat_reset_single_backend_io_counters() we can also remove the need to
> look for the backend type in this function.

Ah, you need that for the pgstat_tracks_io_object() filtering.  I'm
wondering if we should think about making that cheaper at some point.
As a O(N^2) when coupling a call of pg_stat_get_backend_io() with a
scan of pg_stat_activity, perhaps it does not matter much anyway..

Anyway, isn't it possible that this lookup loop finishes by finding
nothing depending on concurrent updates of other beentries?  It sounds
to me that this warrants an early exit in the function.

> BTW, not related to this particular patch but I realized that pgstat_flush_io()
> is called for the walwriter. Indeed, it's coming from the pgstat_report_wal()
> call in WalWriterMain(). That can not report any I/O stats activity (as the
> walwriter is not part of the I/O stats tracking, see pgstat_tracks_io_bktype()).
>
> So it looks like, we could move pgstat_flush_io() outside of pgstat_report_wal()
> and add the pgstat_flush_io() calls only where they need to be made (and so, not
> in WalWriterMain()).

Perhaps, yes.  pgstat_tracks_io_bktype() has always been discarded
walwriters since pgstat_io.c exists.

> But I'm not sure that's worth it until we don't have a need to add more
> pending stats per-backend, thoughts?

Okay with your argument here.

>> +{ oid => '8806', descr => 'statistics: per backend IO statistics',
>> +  proname => 'pg_stat_get_backend_io', prorows => '5', proretset => 't',
>> 
>> Similarly, s/pg_stat_get_backend_io/pg_stat_get_backend_stats/?
> 
> I think that's fine to keep pg_stat_get_backend_io(). If we add more per-backend
> stats in the future then we could add "dedicated" get functions too and a generic
> one retrieving all of them.

Okay about this layer, discarding my remark.  You have a point about
that: other stats associated to a single backend may be OK if not
returned as a SRF, and they'll most likely return a different set of
attributes.

>> +  descr => 'statistics: reset collected IO statistics for a single backend',
>> +  proname => 'pg_stat_reset_single_backend_io_counters', provolatile => 'v', 
>> 
>> And here, pg_stat_reset_backend_stats?
> 
> Same as above, we could imagine that in the future the backend would get mutiple
> stats and that one would want to reset only the I/O ones for example.

Disagreed about this part.  It is slightly simpler to do a full reset
of the stats in a single entry.  If another subset of stats is added
to the backend-level entries, we could always introduce a new function
that has more control over what subset of a single backend entry is
reset.  And I'm pretty sure that we are going to need the function
that does the full reset anyway.

>> I
>> would just add a note in the docs with a query showing how to use it
>> with pg_stat_activity.  An example with LATERAL, doing the same work:
>> select a.pid, s.* from pg_stat_activity as a,
>>    lateral pg_stat_get_backend_io(a.pid) as s
>>   where pid = pg_backend_pid();
> 
> I'm not sure it's worth it. I think that's clear that to get our own stats
> then we need to provide our own backend pid. For example pg_stat_get_activity()
> does not provide such an example using pg_stat_activity or using something like 
> pg_stat_get_activity(pg_backend_pid()).

Okay.

As far as I can see, the patch relies entirely on write_to_file to
prevent any entries to be flushed out.  It means that we leave in the
dshash entries that may sit idle for as long as the server is up once
a pgproc slot is used at least once.  This scales depending on
max_connections.  It also means that we skip the sanity check about
dropped entries at shutdown, which may be a good thing to do because
we don't need to loop through them when writing the stats file.  Hmm.
Could it be better to be more aggressive with the handling of these
stats, marking them as dropped when their backend exists and cleanup
the dshash, without relying on the write flag to make sure that all
the entries are discarded at shutdown?  The point is that we do
shutdown in a controlled manner, with all backends exiting before the
checkpointer writes the stats file after the shutdown checkpoint is
completed.  The patch handles things so as entries are reset when a
procnum is reused, leaving past stats around until that happens.  We
should perhaps aim for more consistency with the way beentry is
refreshed and be more proactive with the backend entry drop or reset
at backend shutdown (pgstat_beshutdown_hook?), so as what is in the
dshash reflects exactly what's in shared memory for each PGPROC and
beentry.

Not sure that the "_per_" added in the various references of the patch
are good to keep, like pgstat_tracks_per_backend_bktype.  These could
be removed, I guess, doing also a PGSTAT_KIND_PER_BACKEND =>
PGSTAT_KIND_BACKEND?
--
Michael

  53. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-12-13T09:20:13Z

    Hi,

On Fri, Dec 13, 2024 at 11:02:53AM +0900, Michael Paquier wrote:
> On Thu, Dec 12, 2024 at 02:02:38PM +0000, Bertrand Drouvot wrote:
> 
> Anyway, isn't it possible that this lookup loop finishes by finding
> nothing depending on concurrent updates of other beentries?  It sounds
> to me that this warrants an early exit in the function.

Right, done that way in the attached.

> Perhaps, yes.  pgstat_tracks_io_bktype() has always been discarded
> walwriters since pgstat_io.c exists.

Yeap. The comment on top of pgstat_tracks_io_bktype() says that it's not done
"for now". I think that we could update the code as proposed until it's done.

> >> +  descr => 'statistics: reset collected IO statistics for a single backend',
> >> +  proname => 'pg_stat_reset_single_backend_io_counters', provolatile => 'v', 
> >> 
> >> And here, pg_stat_reset_backend_stats?
> > 
> > Same as above, we could imagine that in the future the backend would get mutiple
> > stats and that one would want to reset only the I/O ones for example.
> 
> Disagreed about this part.  It is slightly simpler to do a full reset
> of the stats in a single entry.  If another subset of stats is added
> to the backend-level entries, we could always introduce a new function
> that has more control over what subset of a single backend entry is
> reset.  And I'm pretty sure that we are going to need the function
> that does the full reset anyway.

Yeah, would have added it when a new stats subset would be added. It's fine
by me to have it now though, so done that way.

> As far as I can see, the patch relies entirely on write_to_file to
> prevent any entries to be flushed out.

Yes.

> It means that we leave in the
> dshash entries that may sit idle for as long as the server is up once
> a pgproc slot is used at least once.  This scales depending on
> max_connections.  It also means that we skip the sanity check about
> dropped entries at shutdown, which may be a good thing to do because
> we don't need to loop through them when writing the stats file.

Agree.

> Hmm.
> Could it be better to be more aggressive with the handling of these
> stats, marking them as dropped when their backend exists and cleanup
> the dshash, without relying on the write flag to make sure that all
> the entries are discarded at shutdown?

Yeah we can do it to be consistent with other stats kind, done.

> The point is that we do
> shutdown in a controlled manner, with all backends exiting before the
> checkpointer writes the stats file after the shutdown checkpoint is
> completed.  The patch handles things so as entries are reset when a
> procnum is reused, leaving past stats around until that happens.  We
> should perhaps aim for more consistency with the way beentry is
> refreshed and be more proactive with the backend entry drop or reset
> at backend shutdown (pgstat_beshutdown_hook?), so as what is in the
> dshash reflects exactly what's in shared memory for each PGPROC and
> beentry.

That can't be done in pgstat_beshutdown_hook because pgstat_shutdown_hook is called
before and so resets the pgStatLocal.shared_hash during pgstat_detach_shmem().

So, did it in pgstat_shutdown_hook instead. 

> Not sure that the "_per_" added in the various references of the patch
> are good to keep, like pgstat_tracks_per_backend_bktype.  These could
> be removed, I guess, doing also a PGSTAT_KIND_PER_BACKEND =>
> PGSTAT_KIND_BACKEND?

Yeah makes sense, that's consistent with other kinds: done.

Regards,

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

  54. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2024-12-16T08:07:52Z

    On Fri, Dec 13, 2024 at 09:20:13AM +0000, Bertrand Drouvot wrote:
> Yeah makes sense, that's consistent with other kinds: done.

This looks to be taking shape.  I don't have much more comments.

Not feeling so sure about the value brought by the backend_type
returned in pg_stat_get_backend_io(), but well..

+	/* drop the backend stats entry */
+	pgstat_drop_entry(PGSTAT_KIND_BACKEND, InvalidOid, MyProcNumber);

Oh, I've missed something.  Shouldn't pgstat_request_entry_refs_gc()
be called when this returns false?

The creation of the dshash entry is a bit too early, I think..  How
about delaying it more so as we don't create entries that could be
useless if we fail the last steps of authentication?  One spot would
be to delay the creation of the new entry at the end of
pgstat_bestart(), where we know that we are done with authentication
and that the backend is ready to report back to the client connected
to it.  It is true that some subsystems could produce stats as of the 
early transactions they generate, which is why pgstat_initialize() is
done that early in BaseInit(), but that's not really relevant for this
case?

I'm still feeling a bit uneasy about the drop done in
pgstat_shutdown_hook(); it would be nice to make sure that this
happens in a path that would run just after we're done with the
creation of the entry to limit windows where we have an entry but no
way to drop it, or vice-versa, so as the shutdown assertions would
never trigger.  Perhaps there's an argument for an entirely separate
callback that would run before pgstat is plugged off, like a new
before_shmem_exit() callback registered after the entry is created?
--
Michael

  55. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-12-16T15:42:04Z

    Hi,

On Mon, Dec 16, 2024 at 05:07:52PM +0900, Michael Paquier wrote:
> On Fri, Dec 13, 2024 at 09:20:13AM +0000, Bertrand Drouvot wrote:
> 
> Not feeling so sure about the value brought by the backend_type
> returned in pg_stat_get_backend_io(), but well..

It's not necessary per say, but it ensures that pg_stat_get_backend_io() does not
return rows full of "invalid" combinations.

For example:

With the filtering in place, on a "client backend" we would get:

postgres=# select * from pg_stat_get_backend_io(pg_backend_pid());
  backend_type  |    object     |  context  | reads | read_time | writes | write_time | writebacks | writeback_time | extends | extend_time | op_bytes | hits | evictions | reuses | fsyncs | fsync_time | stats_reset
----------------+---------------+-----------+-------+-----------+--------+------------+------------+----------------+---------+-------------+----------+------+-----------+--------+--------+------------+-------------
 client backend | relation      | bulkread  |     0 |         0 |      0 |          0 |          0 |              0 |         |             |     8192 |    0 |         0 |      0 |        |            |
 client backend | relation      | bulkwrite |     0 |         0 |      0 |          0 |          0 |              0 |       0 |           0 |     8192 |    0 |         0 |      0 |        |            |
 client backend | relation      | normal    |    86 |         0 |      0 |          0 |          0 |              0 |       0 |           0 |     8192 | 1604 |         0 |        |      0 |          0 |
 client backend | relation      | vacuum    |     0 |         0 |      0 |          0 |          0 |              0 |       0 |           0 |     8192 |    0 |         0 |      0 |        |            |
 client backend | temp relation | normal    |     0 |         0 |      0 |          0 |            |                |       0 |           0 |     8192 |    0 |         0 |        |        |            |
(5 rows)

and for example for the walsender:

postgres=# select * from pg_stat_get_backend_io(3982910);
 backend_type |    object     |  context  | reads | read_time | writes | write_time | writebacks | writeback_time | extends | extend_time | op_bytes | hits | evictions | reuses | fsyncs | fsync_time | stats_reset
--------------+---------------+-----------+-------+-----------+--------+------------+------------+----------------+---------+-------------+----------+------+-----------+--------+--------+------------+-------------
 walsender    | relation      | bulkread  |     0 |         0 |      0 |          0 |          0 |              0 |         |             |     8192 |    0 |         0 |      0 |        |            |
 walsender    | relation      | bulkwrite |     0 |         0 |      0 |          0 |          0 |              0 |       0 |           0 |     8192 |    0 |         0 |      0 |        |            |
 walsender    | relation      | normal    |     0 |         0 |      0 |          0 |          0 |              0 |       0 |           0 |     8192 |   54 |         0 |        |      0 |          0 |
 walsender    | relation      | vacuum    |     0 |         0 |      0 |          0 |          0 |              0 |       0 |           0 |     8192 |    0 |         0 |      0 |        |            |
 walsender    | temp relation | normal    |     0 |         0 |      0 |          0 |            |                |       0 |           0 |     8192 |    0 |         0 |        |        |            |
(5 rows)

While, without the filtering we would get:

postgres=# select * from pg_stat_get_backend_io(pg_backend_pid());
  backend_type  |    object     |  context  | reads | read_time | writes | write_time | writebacks | writeback_time | extends | extend_time | op_bytes | hits | evictions | reuses | fsyncs | fsync_time | stats_reset
----------------+---------------+-----------+-------+-----------+--------+------------+------------+----------------+---------+-------------+----------+------+-----------+--------+--------+------------+-------------
 client backend | relation      | bulkread  |     0 |         0 |      0 |          0 |          0 |              0 |         |             |     8192 |    0 |         0 |      0 |        |            |
 client backend | relation      | bulkwrite |     0 |         0 |      0 |          0 |          0 |              0 |       0 |           0 |     8192 |    0 |         0 |      0 |        |            |
 client backend | relation      | normal    |     4 |         0 |      0 |          0 |          0 |              0 |       0 |           0 |     8192 |   50 |         0 |        |      0 |          0 |
 client backend | relation      | vacuum    |     0 |         0 |      0 |          0 |          0 |              0 |       0 |           0 |     8192 |    0 |         0 |      0 |        |            |
 client backend | temp relation | bulkread  |       |           |        |            |            |                |         |             |     8192 |      |           |        |        |            |
 client backend | temp relation | bulkwrite |       |           |        |            |            |                |         |             |     8192 |      |           |        |        |            |
 client backend | temp relation | normal    |     0 |         0 |      0 |          0 |            |                |       0 |           0 |     8192 |    0 |         0 |        |        |            |
 client backend | temp relation | vacuum    |       |           |        |            |            |                |         |             |     8192 |      |           |        |        |            |
(8 rows)

and for a walsender:

postgres=# select * from pg_stat_get_backend_io(3981588);
 backend_type |    object     |  context  | reads | read_time | writes | write_time | writebacks | writeback_time | extends | extend_time | op_bytes | hits | evictions | reuses | fsyncs | fsync_time | stats_reset
--------------+---------------+-----------+-------+-----------+--------+------------+------------+----------------+---------+-------------+----------+------+-----------+--------+--------+------------+-------------
 walsender    | relation      | bulkread  |     0 |         0 |      0 |          0 |          0 |              0 |         |             |     8192 |    0 |         0 |      0 |        |            |
 walsender    | relation      | bulkwrite |     0 |         0 |      0 |          0 |          0 |              0 |       0 |           0 |     8192 |    0 |         0 |      0 |        |            |
 walsender    | relation      | normal    |     6 |         0 |      0 |          0 |          0 |              0 |       0 |           0 |     8192 |   48 |         0 |        |      0 |          0 |
 walsender    | relation      | vacuum    |     0 |         0 |      0 |          0 |          0 |              0 |       0 |           0 |     8192 |    0 |         0 |      0 |        |            |
 walsender    | temp relation | bulkread  |       |           |        |            |            |                |         |             |     8192 |      |           |        |        |            |
 walsender    | temp relation | bulkwrite |       |           |        |            |            |                |         |             |     8192 |      |           |        |        |            |
 walsender    | temp relation | normal    |     0 |         0 |      0 |          0 |            |                |       0 |           0 |     8192 |    0 |         0 |        |        |            |
 walsender    | temp relation | vacuum    |       |           |        |            |            |                |         |             |     8192 |      |           |        |        |            |
(8 rows)

It would not be possible to remove those "extra 3 rows" with a join on the
backend type on pg_stat_activity because only the C code knows about what the
compatibility is (though pgstat_tracks_io_object).

> +	/* drop the backend stats entry */
> +	pgstat_drop_entry(PGSTAT_KIND_BACKEND, InvalidOid, MyProcNumber);
> 
> Oh, I've missed something.  Shouldn't pgstat_request_entry_refs_gc()
> be called when this returns false?

I'm not sure that would be possible for a PGSTAT_KIND_BACKEND kind to return
false here but I agree that's better to call pgstat_request_entry_refs_gc()
if that's the case. Done in v9 attached.

> The creation of the dshash entry is a bit too early, I think..  How
> about delaying it more so as we don't create entries that could be
> useless if we fail the last steps of authentication?  One spot would
> be to delay the creation of the new entry at the end of
> pgstat_bestart(), where we know that we are done with authentication
> and that the backend is ready to report back to the client connected
> to it.  It is true that some subsystems could produce stats as of the 
> early transactions they generate, which is why pgstat_initialize() is
> done that early in BaseInit(), but that's not really relevant for this
> case?

I think that makes sense to move the stats entry creation in pgstat_bestart(),
done that way in v9.

> I'm still feeling a bit uneasy about the drop done in
> pgstat_shutdown_hook(); it would be nice to make sure that this
> happens in a path that would run just after we're done with the
> creation of the entry to limit windows where we have an entry but no
> way to drop it, or vice-versa,

I don't believe that's possible as MyProcNumber can't be "reused" (and is
guaranteed to remain valid) until ProcKill() is done (which happens after the
pgstat_shutdown_hook()).

> Perhaps there's an argument for an entirely separate
> callback that would run before pgstat is plugged off, like a new
> before_shmem_exit() callback registered after the entry is created?

As the ProcKill() is run in shmem_exit() (and so after before_shmem_exit()),
I think that the way we currently drop the backend stats entry is fine (unless
I miss something about your concern).

Regards,

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

  56. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2024-12-17T06:26:02Z

    On Mon, Dec 16, 2024 at 03:42:04PM +0000, Bertrand Drouvot wrote:
> It's not necessary per say, but it ensures that pg_stat_get_backend_io() does not
> return rows full of "invalid" combinations.

Okay.  I am not planning to fight more over this point.

> I'm not sure that would be possible for a PGSTAT_KIND_BACKEND kind to return
> false here but I agree that's better to call pgstat_request_entry_refs_gc()
> if that's the case. Done in v9 attached.

It seems to me that it could be possible if a different backend keeps
a reference to this entry.

>> Perhaps there's an argument for an entirely separate
>> callback that would run before pgstat is plugged off, like a new
>> before_shmem_exit() callback registered after the entry is created?
> 
> As the ProcKill() is run in shmem_exit() (and so after before_shmem_exit()),
> I think that the way we currently drop the backend stats entry is fine (unless
> I miss something about your concern).

Looking closely at this one, I think that you're right as long as the
shutdown callback is registered before the entry is created.  So I'm
OK because the drop is a no-op if the entry cannot be found, and we
would not try a pgstat_request_entry_refs_gc().

Anyway, I have put my hands on v9.  A couple of notes, while hacking
through it.  See v9-0002 for the parts I have modified, that applies
on top of your v9-0001.

You may have noticed fee2b3ea2ecdg, which led me to fix two comments
as these paths are not related to only database objects.

Added more documentation, tweaked quite a bit the comments, a bit less
the docs (no need for the two linkends) and applied an indentation.

pg_stat_get_backend_io() can be simpler, and does not need the loop
with the extra LocalPgBackendStatus.  It is possible to call once
pgstat_get_beentry_by_proc_number() and retrieve the PID and the
bktype for the two sanity checks we want to do on them.

s/pgstat_fetch_proc_stat_io/pgstat_fetch_stat_backend/.
s/pgstat_create_backend_stat/pgstat_create_backend/.

I've been wondering for quite a bit about PgStat_BackendPendingIO and
PgStat_PendingIO, and concluded to define both in pgstat.h, with the
former being defined based on the latter to keep the dependency
between both at the same place.
--
Michael

  57. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-12-17T08:13:59Z

    Hi,

On Tue, Dec 17, 2024 at 03:26:02PM +0900, Michael Paquier wrote:
> On Mon, Dec 16, 2024 at 03:42:04PM +0000, Bertrand Drouvot wrote:
> >> Perhaps there's an argument for an entirely separate
> >> callback that would run before pgstat is plugged off, like a new
> >> before_shmem_exit() callback registered after the entry is created?
> > 
> > As the ProcKill() is run in shmem_exit() (and so after before_shmem_exit()),
> > I think that the way we currently drop the backend stats entry is fine (unless
> > I miss something about your concern).
> 
> Looking closely at this one, I think that you're right as long as the
> shutdown callback is registered before the entry is created.

Yeah, which is the case.

> Anyway, I have put my hands on v9.  A couple of notes, while hacking
> through it.  See v9-0002 for the parts I have modified, that applies
> on top of your v9-0001.

Thanks!

> You may have noticed fee2b3ea2ecdg, which led me to fix two comments
> as these paths are not related to only database objects.

Yeap ;-)

> Added more documentation, tweaked quite a bit the comments, a bit less
> the docs (no need for the two linkends) and applied an indentation.

Looks good to me.

> pg_stat_get_backend_io() can be simpler, and does not need the loop
> with the extra LocalPgBackendStatus.  It is possible to call once
> pgstat_get_beentry_by_proc_number() and retrieve the PID and the
> bktype for the two sanity checks we want to do on them.

Looked at the changes you've done and agree that's simpler.

> s/pgstat_fetch_proc_stat_io/pgstat_fetch_stat_backend/.

Not sure about this one as being called in pg_stat_get_backend_io().

> s/pgstat_create_backend_stat/pgstat_create_backend/.

LGTM.

> I've been wondering for quite a bit about PgStat_BackendPendingIO and
> PgStat_PendingIO, and concluded to define both in pgstat.h, with the
> former being defined based on the latter to keep the dependency
> between both at the same place.

That's fine by me.

Also:

=== 1

the changes in pgstat_flush_backend() make sense to me.

=== 2

- * Create the backend statistics entry for procnum.
+ * Create backend statistics entry for proc number.

Yeah, "proc number" looks better as already used in multiple places.

=== 3

the changes in stats.sql look good to me.

Having said that, v9-0002 looks good to me (except the pgstat_fetch_proc_stat_io
renaming).

Regards,

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

  58. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2024-12-17T09:13:44Z

    On Tue, Dec 17, 2024 at 08:13:59AM +0000, Bertrand Drouvot wrote:
> Having said that, v9-0002 looks good to me (except the pgstat_fetch_proc_stat_io
> renaming).

This routine returns a PgStat_Backend from a pgstats entry of type
PGSTAT_KIND_BACKEND, so the name in v9-0001 would not be true once
more types of stats data are attached to this structure.  The name in
v9-0002 is more consistent long-term.
--
Michael

  59. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-12-17T09:35:37Z

    Hi,

On Tue, Dec 17, 2024 at 06:13:44PM +0900, Michael Paquier wrote:
> On Tue, Dec 17, 2024 at 08:13:59AM +0000, Bertrand Drouvot wrote:
> > Having said that, v9-0002 looks good to me (except the pgstat_fetch_proc_stat_io
> > renaming).
> 
> This routine returns a PgStat_Backend from a pgstats entry of type
> PGSTAT_KIND_BACKEND, so the name in v9-0001 would not be true once
> more types of stats data are attached to this structure.

Fair enough, let's go with the name change done in v9-0002.

> The name in
> v9-0002 is more consistent long-term.

Agree, we may need to add parameters to it but we'll see when the time comes.

Regards,

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

  60. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2024-12-18T04:57:53Z

    On Tue, Dec 17, 2024 at 09:35:37AM +0000, Bertrand Drouvot wrote:
> Agree, we may need to add parameters to it but we'll see when the time comes.

Another thing that's been itching me: the loop in
pg_stat_get_backend_io() that exists as well in pg_stat_get_io() looks
worth refactoring in a single routine.  The only difference between
both is the reset timestamp, still both of them can pass a value for
it depending on their stats kind entry.  This shaves a bit more code
in your own patch, even if the check on pgstat_tracks_io_bktype() and
the Assert are not in the inner routine that fills the tuplestore with
the I/O data.  See pg_stat_fill_io_data() in v10-0002.  If you have a
better name for this routine, feel free..

What do you think about this refactoring?  This should come in first,
of course, so as the final patch introducing the backend stats is
easier to parse.
--
Michael

  61. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-12-18T08:11:55Z

    Hi,

On Wed, Dec 18, 2024 at 01:57:53PM +0900, Michael Paquier wrote:
> On Tue, Dec 17, 2024 at 09:35:37AM +0000, Bertrand Drouvot wrote:
> > Agree, we may need to add parameters to it but we'll see when the time comes.
> 
> Another thing that's been itching me: the loop in
> pg_stat_get_backend_io() that exists as well in pg_stat_get_io() looks
> worth refactoring in a single routine.  The only difference between
> both is the reset timestamp, still both of them can pass a value for
> it depending on their stats kind entry.

Yeah, I also had something like this in mind (see R2 in [1]). So, +1 for it.

> This shaves a bit more code
> in your own patch, even if the check on pgstat_tracks_io_bktype() and
> the Assert are not in the inner routine that fills the tuplestore with
> the I/O data.

Yeah, that's fine by me.

> See pg_stat_fill_io_data() in v10-0002.  If you have a
> better name for this routine, feel free..

I think I prefer pg_stat_io_build_tuples() and used that name in v11
attached.

> What do you think about this refactoring?

Makes fully sense to me (as per my first comment above).

> This should come in first,
> of course, so as the final patch introducing the backend stats is
> easier to parse.

Yeah, it's in 0001 attached, with a few changes:

=== 1

s/Save tuples with data from this PgStat_BktypeIO./save tuples with data from this PgStat_BktypeIO/

to be consistent with single line comments around.

=== 2

+  if (stat_reset_timestamp != 0)
+    values[IO_COL_RESET_TIME] = TimestampTzGetDatum(stat_reset_timestamp);
+  else
+    nulls[IO_COL_RESET_TIME] = true;

This is not necessary until the per backend I/O stat is introduced but I 
added it in 0001 to ease 0002 parsing.

=== 3

"In Assert builds, we can afford an extra loop through all of the"

I think that this comment is now confusing since the extra loop would be
done in pg_stat_io_build_tuples() and not where the comment is written. One
option could have been to move the assert and the comment in pg_stat_io_build_tuples()
but I think it's better to have the assert before the pgstat_tracks_io_bktype()
call in pg_stat_get_io(), so modified the comment a bit instead.

0002 is the per-backend stats I/O with yours tweaks.

Regards,

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

  62. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-12-18T08:16:12Z

    On Wed, Dec 18, 2024 at 08:11:55AM +0000, Bertrand Drouvot wrote:
> Yeah, I also had something like this in mind (see R2 in [1]). So, +1 for it.

[1] being: https://www.postgresql.org/message-id/Zy4bmvgHqGjcK1pI%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

  63. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2024-12-19T04:21:54Z

    On Wed, Dec 18, 2024 at 08:11:55AM +0000, Bertrand Drouvot wrote:
> I think I prefer pg_stat_io_build_tuples() and used that name in v11
> attached.

I'll rely to your naming sense than mine, then :D

> I think that this comment is now confusing since the extra loop would be
> done in pg_stat_io_build_tuples() and not where the comment is written. One
> option could have been to move the assert and the comment in pg_stat_io_build_tuples()
> but I think it's better to have the assert before the pgstat_tracks_io_bktype()
> call in pg_stat_get_io(), so modified the comment a bit instead.
> 
> 0002 is the per-backend stats I/O with yours tweaks.

While doing more tests with backends exiting concurrently with their
stats scanned, I have detected one path in pg_stat_get_backend_io()
after calling pgstat_get_beentry_by_proc_number() where we should also
check that it does not return NULL, or we would crash on a pointer
dereference when reading the backend type.

Fixed that, bumped the two version counters, and done.
--
Michael

  64. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-12-19T06:12:04Z

    Hi,

On Thu, Dec 19, 2024 at 01:21:54PM +0900, Michael Paquier wrote:
> 
> While doing more tests with backends exiting concurrently with their
> stats scanned, I have detected one path in pg_stat_get_backend_io()
> after calling pgstat_get_beentry_by_proc_number() where we should also
> check that it does not return NULL, or we would crash on a pointer
> dereference when reading the backend type.
> 
> Fixed that,

Oh right, indeed all the others pgstat_get_beentry_by_proc_number() callers are
checking for a NULL returned value.

> bumped the two version counters, and done.

Thanks!

I think I'll start a dedicated thread to discuss the stats_fetch_consistency/'snapshot'
point (will be easier to follow than resuming the discussion in this thread).

Regards,

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

  65. Re: per backend I/O statistics

    Alexander Law <exclusion@gmail.com> — 2024-12-20T06:00:00Z

    Hello Michael,

19.12.2024 06:21, Michael Paquier wrote:
> Fixed that, bumped the two version counters, and done.

Could you, please, look at recent failures produced by grassquit (which
has fsync = on in it's config), on an added test case? For instance, [1]:
--- /home/bf/bf-build/grassquit/HEAD/pgsql/src/test/regress/expected/stats.out 2024-12-19 04:44:08.779311933 +0000
+++ /home/bf/bf-build/grassquit/HEAD/pgsql.build/testrun/recovery/027_stream_regress/data/results/stats.out 2024-12-19 
16:37:41.351784840 +0000
@@ -1333,7 +1333,7 @@
        AND :my_io_sum_shared_after_fsyncs= 0);
   ?column?
  ----------
- t
+ f
  (1 row)

The complete query is:
SELECT current_setting('fsync') = 'off'
   OR (:my_io_sum_shared_after_fsyncs = :my_io_sum_shared_before_fsyncs
       AND :my_io_sum_shared_after_fsyncs= 0);

And the corresponding query in 027_stream_regress_primary.log is:
2024-12-19 16:37:39.907 UTC [4027467][client backend][15/1980:0] LOG:  statement: SELECT current_setting('fsync') = 'off'
       OR (1 = 1
           AND 1= 0);

(I can reproduce this locally with an asan-enabled build too.)

[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=grassquit&dt=2024-12-19%2016%3A28%3A58

Best regards,
Alexander

  66. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-12-20T09:09:00Z

    Hi,

On Fri, Dec 20, 2024 at 08:00:00AM +0200, Alexander Lakhin wrote:
> Hello Michael,
> 
> 19.12.2024 06:21, Michael Paquier wrote:
> > Fixed that, bumped the two version counters, and done.
> 
> Could you, please, look at recent failures produced by grassquit (which
> has fsync = on in it's config), on an added test case? For instance, [1]:
> --- /home/bf/bf-build/grassquit/HEAD/pgsql/src/test/regress/expected/stats.out 2024-12-19 04:44:08.779311933 +0000
> +++ /home/bf/bf-build/grassquit/HEAD/pgsql.build/testrun/recovery/027_stream_regress/data/results/stats.out
> 2024-12-19 16:37:41.351784840 +0000
> @@ -1333,7 +1333,7 @@
>        AND :my_io_sum_shared_after_fsyncs= 0);
>   ?column?
>  ----------
> - t
> + f
>  (1 row)
> 
> The complete query is:
> SELECT current_setting('fsync') = 'off'
>   OR (:my_io_sum_shared_after_fsyncs = :my_io_sum_shared_before_fsyncs
>       AND :my_io_sum_shared_after_fsyncs= 0);
> 
> And the corresponding query in 027_stream_regress_primary.log is:
> 2024-12-19 16:37:39.907 UTC [4027467][client backend][15/1980:0] LOG:  statement: SELECT current_setting('fsync') = 'off'
>       OR (1 = 1
>           AND 1= 0);
> 
> (I can reproduce this locally with an asan-enabled build too.)
> 
> [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=grassquit&dt=2024-12-19%2016%3A28%3A58

Thanks for the report! I was not able able to reproduce (even with asan-enabled)
but I think the test is wrong. Indeed the backend could fsync too during the test
(see register_dirty_segment() and the case where the request queue is full).

I think the test should look like the attached instead, thoughts?

Regards,

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

  67. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2024-12-20T09:24:37Z

    On Fri, Dec 20, 2024 at 09:09:00AM +0000, Bertrand Drouvot wrote:
> Thanks for the report! I was not able able to reproduce (even with asan-enabled)
> but I think the test is wrong. Indeed the backend could fsync too during the test
> (see register_dirty_segment() and the case where the request queue is full).
> 
> I think the test should look like the attached instead, thoughts?

Hmm.  I cannot reproduce that here either.  FWIW, I use these settings
by default in my local builds, similarly to the CI which did not
complain:
ASAN_OPTIONS="print_stacktrace=1:disable_coredump=0:abort_on_error=1:detect_leaks=0"
UBSAN_OPTIONS="print_stacktrace=1:disable_coredump=0:abort_on_error=1:verbosity=2"
CFLAGS+="-fsanitize=undefined "
LDFLAGS+="-fsanitize=undefined "

grassquit uses something a bit different, which don't allow me to see
a problem with 027 even with fsync enabled:
ASAN_OPTIONS="print_stacktrace=1:disable_coredump=0:abort_on_error=1:detect_leaks=0:detect_stack_use_after_return=0"
CFLAGS+="-fsanitize=address -fno-sanitize-recover=all "
LDFLAGS+="-fsanitize=address -fno-sanitize-recover=all "

Anyway, I was arriving at the same conclusion as you, because this is
just telling us that there could be "some" sync activity.  As
rewritten, this would just check that the after-the-fact counter is
never lower than the before-the-fact counter, which is still better
than removing the query.  I was thinking about doing the latter and
remove the query, but I'm also OK with what you have here, keeping the
query with a relaxed check.  I'll go adjust that in a bit..
--
Michael

  68. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-12-20T09:57:19Z

    Hi,

On Thu, Dec 19, 2024 at 06:12:04AM +0000, Bertrand Drouvot wrote:
> On Thu, Dec 19, 2024 at 01:21:54PM +0900, Michael Paquier wrote:
> > bumped the two version counters, and done.

> >    The existing structure could be expanded in the
> >    future to add more information about other statistics related to
> >    backends, depending on requirements or ideas.

BTW, now that the per backend I/O statistics is done, I'll start working on per
backend wal statistics.

Regards,

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

  69. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2024-12-24T05:35:16Z

    On Fri, Dec 20, 2024 at 09:57:19AM +0000, Bertrand Drouvot wrote:
> BTW, now that the per backend I/O statistics is done, I'll start working on per
> backend wal statistics.

I think that this is a good idea.  It does not actually overlap the
proposal in [1] as the stats persistency is not the same.  What this
thread has taught me is that you could just plug in the stats of the
new backend-level structure a PgStat_WalStats and retrieve them with a
new function that returns a single tuple with the WAL stats
attributes.  That should be rather straight-forward to achieve.

[1]: https://commitfest.postgresql.org/51/4950/
--
Michael

  70. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-01-03T10:48:41Z

    Hi,

On Tue, Dec 24, 2024 at 02:35:16PM +0900, Michael Paquier wrote:
> On Fri, Dec 20, 2024 at 09:57:19AM +0000, Bertrand Drouvot wrote:
> > BTW, now that the per backend I/O statistics is done, I'll start working on per
> > backend wal statistics.
> 
> I think that this is a good idea.

Thanks for sharing your thoughts.

> It does not actually overlap the
> proposal in [1] as the stats persistency is not the same.  What this
> thread has taught me is that you could just plug in the stats of the
> new backend-level structure a PgStat_WalStats and retrieve them with a
> new function that returns a single tuple with the WAL stats
> attributes.  That should be rather straight-forward to achieve.

I started to look at it and should be able to share a patch next week.

Regards,

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

  71. Re: per backend I/O statistics

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

    Hi,

On Fri, Jan 03, 2025 at 10:48:41AM +0000, Bertrand Drouvot wrote:
> I started to look at it and should be able to share a patch next week.

A dedicated thread and a patch have been created, see [1].

[1]: https://www.postgresql.org/message-id/flat/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

  72. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2025-01-07T23:54:40Z

    On Tue, Jan 07, 2025 at 08:54:59AM +0000, Bertrand Drouvot wrote:
> A dedicated thread and a patch have been created, see [1].
> 
> [1]: https://www.postgresql.org/message-id/flat/Z3zqc4o09dM/Ezyz%40ip-10-97-1-34.eu-west-3.compute.internal

Hmm.  I can see that you have some refactoring pieces in the lines of
some stuff I wanted to structure for the sake of this thread, but I
have discarded them to simplify the final result at the end and it
did not change much.

Will look at all that on this other thread, no need to start a
separate thread just for the sake of the refactoring bits.
--
Michael

  73. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-01-10T13:56:01Z

    Hi,

On Thu, Dec 19, 2024 at 06:12:04AM +0000, Bertrand Drouvot wrote:
> I think I'll start a dedicated thread to discuss the stats_fetch_consistency/'snapshot'
> point (will be easier to follow than resuming the discussion in this thread).

I gave more thoughts on it and did some research in the mailing list.

I've read those comments, from [1]:

"
We had a lot of discussion around what consistency
model we want, and Tom was
adamant that there needs to be a mode that behaves
like the current consistency
model (which is what snapshot behaves like, with
very minor differences)
"

and from [2]:

"
One thing we definitely need to add documentation for is the
stats_fetch_consistency GUC. I think we should change its default to 'cache',
because that still gives the ability to "self-join", without the cost of the
current method.
"

So now, I think that it is not a good idea for the per backend stats to 
behave differently than the other stats kinds. I think we should get rid of
the "snapshot" for all of them or for none of them. Reading the above comments,
it looks like it has already been discussed and that the outcome is to keep
it for all of them.

Regards,

[1]: https://www.postgresql.org/message-id/20220404210610.6dgbwzpbiveyl5y3%40alap3.anarazel.de
[2]: https://www.postgresql.org/message-id/20220329072651.imjqtzxwk6xycojv%40alap3.anarazel.de

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

  74. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2025-01-14T04:56:13Z

    On Fri, Jan 10, 2025 at 01:56:01PM +0000, Bertrand Drouvot wrote:
> So now, I think that it is not a good idea for the per backend stats to 
> behave differently than the other stats kinds. I think we should get rid of
> the "snapshot" for all of them or for none of them. Reading the above comments,
> it looks like it has already been discussed and that the outcome is to keep
> it for all of them.

Thanks a lot for diving into these details!  I did not recall that
there was a strict argument for the current behavior, even if I was
pretty sure that introducing an exception for the backend stats was
not quite right compared to the other stats kinds.  The consistency
line prevails here, then.
--
Michael

  75. Re: per backend I/O statistics

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2025-01-15T08:03:54Z

    Hi,

On Thu, 19 Dec 2024 at 07:22, Michael Paquier <michael@paquier.xyz> wrote:
>
> Fixed that, bumped the two version counters, and done.

I encountered a problem while trying to add WAL stats to pg_stat_io
and I wanted to hear your thoughts.

Right now, pgstat_prep_backend_pending() is called in both
pgstat_count_io_op() and pgstat_count_io_op_time() to create a local
PgStat_BackendPending entry. In that process,
pgstat_prep_pending_entry() -> MemoryContextAllocZero() is called. The
problem is that MemoryContextAllocZero() can not be called in the
critical sections.

For example, here is what happens in the walsender backend:

'''
... ->
exec_replication_command() ->
SendBaseBackup() ->
...  ->
XLogInsertRecord() ->
START_CRIT_SECTION() /* Now we are in the critical section */ ->
... ->
XLogWrite() ->
pgstat_count_io_op_time() for the pg_pwrite() IO ->
pgstat_prep_backend_pending() ->
pgstat_prep_pending_entry() ->
MemoryContextAllocZero() ->
Failed at Assert("CritSectionCount == 0 || (context)->allowInCritSection")
'''

With this commit it may not be possible to count IOs in the critical
sections. I think the problem happens only if the local
PgStat_BackendPending entry is being created for the first time for
this backend in the critical section.

--
Regards,
Nazir Bilal Yavuz
Microsoft

  76. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-01-15T08:30:20Z

    Hi,

On Wed, Jan 15, 2025 at 11:03:54AM +0300, Nazir Bilal Yavuz wrote:
> Hi,
> 
> On Thu, 19 Dec 2024 at 07:22, Michael Paquier <michael@paquier.xyz> wrote:
> >
> > Fixed that, bumped the two version counters, and done.
> 
> I encountered a problem while trying to add WAL stats to pg_stat_io
> and I wanted to hear your thoughts.
> 
> Right now, pgstat_prep_backend_pending() is called in both
> pgstat_count_io_op() and pgstat_count_io_op_time() to create a local
> PgStat_BackendPending entry. In that process,
> pgstat_prep_pending_entry() -> MemoryContextAllocZero() is called. The
> problem is that MemoryContextAllocZero() can not be called in the
> critical sections.
> 
> For example, here is what happens in the walsender backend:
> 
> '''
> ... ->
> exec_replication_command() ->
> SendBaseBackup() ->
> ...  ->
> XLogInsertRecord() ->
> START_CRIT_SECTION() /* Now we are in the critical section */ ->
> ... ->
> XLogWrite() ->
> pgstat_count_io_op_time() for the pg_pwrite() IO ->
> pgstat_prep_backend_pending() ->
> pgstat_prep_pending_entry() ->
> MemoryContextAllocZero() ->
> Failed at Assert("CritSectionCount == 0 || (context)->allowInCritSection")
> '''
> 
> With this commit it may not be possible to count IOs in the critical
> sections. I think the problem happens only if the local
> PgStat_BackendPending entry is being created for the first time for
> this backend in the critical section.

Yeah, I encountered the exact same thing and mentioned it in [1] (see R1.).

In [1] I did propose to use a new PendingBackendWalStats variable to "bypass" the
pgstat_prep_backend_pending() usage.

Michael mentioned in [2] that is not really consistent with the rest (what I
agree with) and that "we should rethink a bit the way pending entries are
retrieved". I did not think about it yet but that might be the way to
go, thoughts? 

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

Regards,

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

  77. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2025-01-15T09:27:22Z

    On Wed, Jan 15, 2025 at 08:30:20AM +0000, Bertrand Drouvot wrote:
> On Wed, Jan 15, 2025 at 11:03:54AM +0300, Nazir Bilal Yavuz wrote:
>> With this commit it may not be possible to count IOs in the critical
>> sections. I think the problem happens only if the local
>> PgStat_BackendPending entry is being created for the first time for
>> this backend in the critical section.
> 
> Yeah, I encountered the exact same thing and mentioned it in [1] (see R1.).
> 
> In [1] I did propose to use a new PendingBackendWalStats variable to "bypass" the
> pgstat_prep_backend_pending() usage.
> 
> Michael mentioned in [2] that is not really consistent with the rest (what I
> agree with) and that "we should rethink a bit the way pending entries are
> retrieved". I did not think about it yet but that might be the way to
> go, thoughts? 
> 
> [1]: https://www.postgresql.org/message-id/Z3zqc4o09dM/Ezyz%40ip-10-97-1-34.eu-west-3.compute.internal
> [2]: https://www.postgresql.org/message-id/Z4dRlNuhSQ3hPPv2%40paquier.xyz

My problem is that this is not only related to backend stats, but to
all variable-numbered stats kinds that require this behavior.  One
other case where I've seen this as being an issue is injection point
stats, for example, while enabling these stats for all callbacks and
some of them are run in critical sections.

A generic solution to the problem would be to allow
pgStatPendingContext to have MemoryContextAllowInCriticalSection() set
temporarily for the allocation of the pending data.
Perhaps not for all stats kinds, just for these where we're OK with
the potential memory footprint so we could have a flag in
PgStat_KindInfo.  Giving to all stats kinds the responsibility to
allocate a pending entry beforehand outside a critical section is
another option, but that means going through different tweaks for all
stats kinds that require them.

Andres, do you have any thoughts about that?  Not sure what would be 
your view on this matter.  MemoryContextAllowInCriticalSection() for
stats kinds that allow the case would be OK for me I guess, we should
not be talking about a lot of memory.
--
Michael

  78. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-01-15T10:55:44Z

    Hi,

On Wed, Jan 15, 2025 at 06:27:22PM +0900, Michael Paquier wrote:
> On Wed, Jan 15, 2025 at 08:30:20AM +0000, Bertrand Drouvot wrote:
> > On Wed, Jan 15, 2025 at 11:03:54AM +0300, Nazir Bilal Yavuz wrote:
> >> With this commit it may not be possible to count IOs in the critical
> >> sections. I think the problem happens only if the local
> >> PgStat_BackendPending entry is being created for the first time for
> >> this backend in the critical section.
> > 
> > Yeah, I encountered the exact same thing and mentioned it in [1] (see R1.).
> > 
> > In [1] I did propose to use a new PendingBackendWalStats variable to "bypass" the
> > pgstat_prep_backend_pending() usage.
> > 
> > Michael mentioned in [2] that is not really consistent with the rest (what I
> > agree with) and that "we should rethink a bit the way pending entries are
> > retrieved". I did not think about it yet but that might be the way to
> > go, thoughts? 
> > 
> > [1]: https://www.postgresql.org/message-id/Z3zqc4o09dM/Ezyz%40ip-10-97-1-34.eu-west-3.compute.internal
> > [2]: https://www.postgresql.org/message-id/Z4dRlNuhSQ3hPPv2%40paquier.xyz
> 
> My problem is that this is not only related to backend stats, but to
> all variable-numbered stats kinds that require this behavior.

Yeah, agree that it's better to have a "generic" solution.

> A generic solution to the problem would be to allow
> pgStatPendingContext to have MemoryContextAllowInCriticalSection() set
> temporarily for the allocation of the pending data.
> Perhaps not for all stats kinds, just for these where we're OK with
> the potential memory footprint so we could have a flag in
> PgStat_KindInfo.  Giving to all stats kinds the responsibility to
> allocate a pending entry beforehand outside a critical section is
> another option, but that means going through different tweaks for all
> stats kinds that require them.
> 
> Andres, do you have any thoughts about that?  Not sure what would be 
> your view on this matter.  MemoryContextAllowInCriticalSection() for
> stats kinds that allow the case would be OK for me I guess,

Same here, that looks ok to me too.

Regards,

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

  79. Re: per backend I/O statistics

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2025-01-15T14:20:57Z

    Hi,

On Wed, 15 Jan 2025 at 12:27, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Jan 15, 2025 at 08:30:20AM +0000, Bertrand Drouvot wrote:
> > On Wed, Jan 15, 2025 at 11:03:54AM +0300, Nazir Bilal Yavuz wrote:
> >> With this commit it may not be possible to count IOs in the critical
> >> sections. I think the problem happens only if the local
> >> PgStat_BackendPending entry is being created for the first time for
> >> this backend in the critical section.
> >
> > Yeah, I encountered the exact same thing and mentioned it in [1] (see R1.).
> >
> > In [1] I did propose to use a new PendingBackendWalStats variable to "bypass" the
> > pgstat_prep_backend_pending() usage.
> >
> > Michael mentioned in [2] that is not really consistent with the rest (what I
> > agree with) and that "we should rethink a bit the way pending entries are
> > retrieved". I did not think about it yet but that might be the way to
> > go, thoughts?
> >
> > [1]: https://www.postgresql.org/message-id/Z3zqc4o09dM/Ezyz%40ip-10-97-1-34.eu-west-3.compute.internal
> > [2]: https://www.postgresql.org/message-id/Z4dRlNuhSQ3hPPv2%40paquier.xyz

Thanks! I must have missed these emails.

>
> My problem is that this is not only related to backend stats, but to
> all variable-numbered stats kinds that require this behavior.  One
> other case where I've seen this as being an issue is injection point
> stats, for example, while enabling these stats for all callbacks and
> some of them are run in critical sections.
>
> A generic solution to the problem would be to allow
> pgStatPendingContext to have MemoryContextAllowInCriticalSection() set
> temporarily for the allocation of the pending data.
> Perhaps not for all stats kinds, just for these where we're OK with
> the potential memory footprint so we could have a flag in
> PgStat_KindInfo.  Giving to all stats kinds the responsibility to
> allocate a pending entry beforehand outside a critical section is
> another option, but that means going through different tweaks for all
> stats kinds that require them.

I think allowing only pgStatPendingContext to have
MemoryContextAllowInCriticalSection() is not enough. We need to allow
at least pgStatSharedRefContext as well to have
MemoryContextAllowInCriticalSection() as it can be allocated too.

'''
pgstat_prep_pending_entry() ->
pgstat_get_entry_ref() ->
pgstat_get_entry_ref_cached() ->
MemoryContextAlloc(pgStatSharedRefContext, sizeof(PgStat_EntryRef))
'''

Also, I encountered another problem. I did not write it in the first
email because I thought I fixed it but it seems I did not.

While doing the initdb, we are restoring stats with the
pgstat_restore_stats() and we do not expect any pending stats. The
problem goes like that:

'''
pgstat_restore_stats() ->
pgstat_read_statsfile() ->
pgstat_reset_after_failure() ->
pgstat_drop_all_entries() ->
pgstat_drop_entry_internal() ->
We have an assertion there which checks if there is a pending stat entry:

    /* should already have released local reference */
    if (pgStatEntryRefHash)
        Assert(!pgstat_entry_ref_hash_lookup(pgStatEntryRefHash, shent->key));
'''

And we have this at the same time:

'''
BootstrapModeMain() ->
InitPostgres() ->
StartupXLOG() ->
ReadCheckpointRecord() ->
InitWalRecovery() ->
... ->
XLogReadAhead() ->
XLogDecodeNextRecord() ->
ReadPageInternal() ->
state->routine.page_read = XLogPageRead() then WAL read happens
'''

So, this assert fails because we have pending stats for the
PGSTAT_KIND_BACKEND. My fix was simply not restoring stats when the
bootstrap is happening; but it did not work. Because, we reset all
fixed-numbered stats 'pgstat_reset_after_failure() ->
kind_info->reset_all_cb()' there, so if we skip it; we have some NULL
stats, for example reset timestamps. Some of the tests do not expect
them to be NULL at the start like below:

SELECT stats_reset AS archiver_reset_ts FROM pg_stat_archiver \gset
SELECT pg_stat_reset_shared('archiver');
SELECT stats_reset > :'archiver_reset_ts'::timestamptz FROM pg_stat_archiver;

This test fails because archiver_reset_ts is NULL when the server is started.

I was a bit surprised that Bertrand did not encounter the same problem
while working on the 'per backend WAL statistics' patch. Then I found
the reason, it is because this problem happens only for WAL read and
WAL init IOs which are starting to be tracked in my patch. By saying
that, I could not decide which thread to write about this problem,
migrating WAL stats thread or this thread. Since this thread is active
and other stats may cause the same problem, I decided to write here.
Please warn me if you think I need to write this to the migrating WAL
stats thread.

[1]
diff --git a/src/backend/access/transam/xlog.c
b/src/backend/access/transam/xlog.c
index bf3dbda901d..0538859cc7f 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -5681,7 +5681,7 @@ StartupXLOG(void)
      */
     if (didCrash)
         pgstat_discard_stats();
-    else
+    else if (!IsBootstrapProcessingMode())
         pgstat_restore_stats(checkPoint.redo);

--
Regards,
Nazir Bilal Yavuz
Microsoft

  80. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-01-15T16:00:12Z

    Hi,

On Wed, Jan 15, 2025 at 05:20:57PM +0300, Nazir Bilal Yavuz wrote:
> While doing the initdb, we are restoring stats with the
> pgstat_restore_stats() and we do not expect any pending stats. The
> problem goes like that:
> 
> I was a bit surprised that Bertrand did not encounter the same problem
> while working on the 'per backend WAL statistics' patch. Then I found
> the reason, it is because this problem happens only for WAL read and
> WAL init IOs which are starting to be tracked in my patch. By saying
> that, I could not decide which thread to write about this problem,
> migrating WAL stats thread or this thread. Since this thread is active
> and other stats may cause the same problem, I decided to write here.
> Please warn me if you think I need to write this to the migrating WAL
> stats thread.

Thanks for the report! As it looks like that you hit the issue for "WAL read and
WAL init IOs which are starting to be tracked in my patch", then I think it would
make more sense to mention the issue in the "migrating WAL stats thread" so that
one could try to reproduce it (with a patch producing the issue at hands).

So it seems to me that the "migrating WAL stats thread" is a better place to
discuss about it.

Regards,

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

  81. Re: per backend I/O statistics

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

    On Wed, Jan 15, 2025 at 05:20:57PM +0300, Nazir Bilal Yavuz wrote:
> I think allowing only pgStatPendingContext to have
> MemoryContextAllowInCriticalSection() is not enough. We need to allow
> at least pgStatSharedRefContext as well to have
> MemoryContextAllowInCriticalSection() as it can be allocated too.
> 
> '''
> pgstat_prep_pending_entry() ->
> pgstat_get_entry_ref() ->
> pgstat_get_entry_ref_cached() ->
> MemoryContextAlloc(pgStatSharedRefContext, sizeof(PgStat_EntryRef))
> '''

Yep, I was pretty sure that we have a bit more going on.  Last time I
began digging into the issue I was loading injection_points in
shared_preload_libraries with stats enabled to see how much I could
break, and this was one pattern once I've forced the pending part.  I
didn't get through the whole exercise.

> While doing the initdb, we are restoring stats with the
> pgstat_restore_stats() and we do not expect any pending stats. The
> problem goes like that:
> 
> '''
> pgstat_restore_stats() ->
> pgstat_read_statsfile() ->
> pgstat_reset_after_failure() ->
> pgstat_drop_all_entries() ->
> pgstat_drop_entry_internal() ->
> We have an assertion there which checks if there is a pending stat entry:
> 
>     /* should already have released local reference */
>     if (pgStatEntryRefHash)
>         Assert(!pgstat_entry_ref_hash_lookup(pgStatEntryRefHash, shent->key));
> '''

You mean that this is not something that happens on HEAD, but as an
effect of trying to add WAL stats to pg_stat_io, right?

> I was a bit surprised that Bertrand did not encounter the same problem
> while working on the 'per backend WAL statistics' patch. Then I found
> the reason, it is because this problem happens only for WAL read and
> WAL init IOs which are starting to be tracked in my patch. By saying
> that, I could not decide which thread to write about this problem,
> migrating WAL stats thread or this thread. Since this thread is active
> and other stats may cause the same problem, I decided to write here.
> Please warn me if you think I need to write this to the migrating WAL
> stats thread.

I'd rather treat that on a separate thread (please add me in CC if I'm
not there yet!).  I am OK to discuss any issues related to backend
statistics here if HEAD is impacted based on how the feature is
limited currently now, though.

Another, simpler, idea would be to force more calls
pgstat_prep_backend_pending() where this is problematic, like before
entering a critical section when generating a WAL record.  I am not
sure that it would take care of everything, as you mention; that would
leave some holes depending on the kind of interactions with the
pgstats dshash we do in these paths.
--
Michael

  82. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-01-16T06:48:58Z

    Hi,

On Thu, Jan 16, 2025 at 09:55:10AM +0900, Michael Paquier wrote:
> On Wed, Jan 15, 2025 at 05:20:57PM +0300, Nazir Bilal Yavuz wrote:
> > I think allowing only pgStatPendingContext to have
> > MemoryContextAllowInCriticalSection() is not enough. We need to allow
> > at least pgStatSharedRefContext as well to have
> > MemoryContextAllowInCriticalSection() as it can be allocated too.
> > 
> > '''
> > pgstat_prep_pending_entry() ->
> > pgstat_get_entry_ref() ->
> > pgstat_get_entry_ref_cached() ->
> > MemoryContextAlloc(pgStatSharedRefContext, sizeof(PgStat_EntryRef))
> > '''
> 
> Yep, I was pretty sure that we have a bit more going on.  Last time I
> began digging into the issue I was loading injection_points in
> shared_preload_libraries with stats enabled to see how much I could
> break, and this was one pattern once I've forced the pending part.  I
> didn't get through the whole exercise.

I'll look at it and come back with a proposal as part of [1].

[1]: https://www.postgresql.org/message-id/Z4dRlNuhSQ3hPPv2@paquier.xyz

Regards,

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

  83. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-01-16T16:05:35Z

    Hi,

On Thu, Jan 16, 2025 at 06:48:58AM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Thu, Jan 16, 2025 at 09:55:10AM +0900, Michael Paquier wrote:
> > On Wed, Jan 15, 2025 at 05:20:57PM +0300, Nazir Bilal Yavuz wrote:
> > > I think allowing only pgStatPendingContext to have
> > > MemoryContextAllowInCriticalSection() is not enough. We need to allow
> > > at least pgStatSharedRefContext as well to have
> > > MemoryContextAllowInCriticalSection() as it can be allocated too.
> > > 
> > > '''
> > > pgstat_prep_pending_entry() ->
> > > pgstat_get_entry_ref() ->
> > > pgstat_get_entry_ref_cached() ->
> > > MemoryContextAlloc(pgStatSharedRefContext, sizeof(PgStat_EntryRef))
> > > '''
> > 
> > Yep, I was pretty sure that we have a bit more going on.  Last time I
> > began digging into the issue I was loading injection_points in
> > shared_preload_libraries with stats enabled to see how much I could
> > break, and this was one pattern once I've forced the pending part.  I
> > didn't get through the whole exercise.
> 
> I'll look at it and come back with a proposal as part of [1].

I looked at it and shared the outcome in [1]. It looks like it's more complicated
as we could also hit a failed assertion in MemoryContextCreate(), 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]

I propose that as of now we discuss the issue in [1] instead of here.

[1]: https://www.postgresql.org/message-id/Z4ks4%2BwnAz8eEyX9%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

  84. Re: per backend I/O statistics

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

    Hi,

On 2025-01-15 18:27:22 +0900, Michael Paquier wrote:
> On Wed, Jan 15, 2025 at 08:30:20AM +0000, Bertrand Drouvot wrote:
> > On Wed, Jan 15, 2025 at 11:03:54AM +0300, Nazir Bilal Yavuz wrote:
> >> With this commit it may not be possible to count IOs in the critical
> >> sections. I think the problem happens only if the local
> >> PgStat_BackendPending entry is being created for the first time for
> >> this backend in the critical section.
> >
> > Yeah, I encountered the exact same thing and mentioned it in [1] (see R1.).
> >
> > In [1] I did propose to use a new PendingBackendWalStats variable to "bypass" the
> > pgstat_prep_backend_pending() usage.
> >
> > Michael mentioned in [2] that is not really consistent with the rest (what I
> > agree with) and that "we should rethink a bit the way pending entries are
> > retrieved". I did not think about it yet but that might be the way to
> > go, thoughts?
> >
> > [1]: https://www.postgresql.org/message-id/Z3zqc4o09dM/Ezyz%40ip-10-97-1-34.eu-west-3.compute.internal
> > [2]: https://www.postgresql.org/message-id/Z4dRlNuhSQ3hPPv2%40paquier.xyz
>
> My problem is that this is not only related to backend stats, but to
> all variable-numbered stats kinds that require this behavior.

The issue here imo is that recently IO stats were turned into a variable
numbered stat, where it previously wasn't.  It seems like a bad idea that now
we regularly do a somewhat sizable memory allocation for backend stats. For
table stats allocating the pending memory is somewhat a necessity - there
could be a lot of different tables after all. And later transactions won't
necessarily access the same set of tables, so we can't just keep the memory
for pending around - but that's not true for IO.


> One other case where I've seen this as being an issue is injection point
> stats, for example, while enabling these stats for all callbacks and some of
> them are run in critical sections.

That seems somewhat fundamental - you could have arbitrary numbers of
injection points and thus need arbitrary amounts of memory. You can't just do
that in a critical section. And it's not just about a potential memory
allocation for pending stats, the DSM allocation can't be done in a critical
section either.


> A generic solution to the problem would be to allow
> pgStatPendingContext to have MemoryContextAllowInCriticalSection() set
> temporarily for the allocation of the pending data.
> Perhaps not for all stats kinds, just for these where we're OK with
> the potential memory footprint so we could have a flag in
> PgStat_KindInfo.

That seems like an *incredibly* bad idea. That restriction exists for a
reason.


> Andres, do you have any thoughts about that?  Not sure what would be
> your view on this matter.  MemoryContextAllowInCriticalSection() for
> stats kinds that allow the case would be OK for me I guess, we should
> not be talking about a lot of memory.

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. And for something like
injection points (not really convinced that it's a good thing to have stats
for, but ...), allocation the shared memory stats object outside of the
critical section and not having any pending stats seems to be the right
approach.

Greetings,

Andres Freund

  85. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2025-01-17T00:08:02Z

    On Thu, Jan 16, 2025 at 11:28:43AM -0500, Andres Freund wrote:
> On 2025-01-15 18:27:22 +0900, Michael Paquier wrote:
>> My problem is that this is not only related to backend stats, but to
>> all variable-numbered stats kinds that require this behavior.
> 
> The issue here imo is that recently IO stats were turned into a variable
> numbered stat, where it previously wasn't. It seems like a bad idea that now
> we regularly do a somewhat sizable memory allocation for backend stats. For
> table stats allocating the pending memory is somewhat a necessity - there
> could be a lot of different tables after all. And later transactions won't
> necessarily access the same set of tables, so we can't just keep the memory
> for pending around - but that's not true for IO.

Yep, for the reason that backend stats require that with one entry in
the pgstats dshash for each backend as these are sized depending on
max_connections & co using a proc number for the hash key.

>> Andres, do you have any thoughts about that?  Not sure what would be
>> your view on this matter.  MemoryContextAllowInCriticalSection() for
>> stats kinds that allow the case would be OK for me I guess, we should
>> not be talking about a lot of memory.
> 
> 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. And for something like
> injection points (not really convinced that it's a good thing to have stats
> for, but ...), allocation the shared memory stats object outside of the
> critical section and not having any pending stats seems to be the right
> approach.

Not sure to completely agree on that as this pushes the responsibility
of how to deal with the critical section handling to each stats kind,
but I won't fight over it for this case, either.  It would not be the
first time we have specific logic to do allocations beforehand when
dealing with critical sections (38c579b08988 is the most recent
example coming in mind, there are many others). 

Anyway, let's just switch pgstat_backend.c so as we use a static
PgStat_BackendPending (pending name) to store the pending IO stats
that could be reused for also the WAL bits.  It does not seem that
complicated as far as I can see, removing pgstat_prep_backend_pending
and changing pgstat_flush_backend_entry_io to use the static pending
data.  The patch set posted here is doing that within its
pgstat_flush_backend_entry_wal():
https://www.postgresql.org/message-id/Z4DrFhdlO6Xf13Xd@ip-10-97-1-34.eu-west-3.compute.internal

I could tweak that around the beginning of next week with a proposal
of patch.  Bertrand, perhaps you'd prefer hack on this one?
--
Michael

  86. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-01-17T06:06:35Z

    Hi,

On Fri, Jan 17, 2025 at 09:08:02AM +0900, Michael Paquier wrote:
> Anyway, let's just switch pgstat_backend.c so as we use a static
> PgStat_BackendPending (pending name) to store the pending IO stats
> that could be reused for also the WAL bits.  It does not seem that
> complicated as far as I can see, removing pgstat_prep_backend_pending
> and changing pgstat_flush_backend_entry_io to use the static pending
> data.  The patch set posted here is doing that within its
> pgstat_flush_backend_entry_wal():
> https://www.postgresql.org/message-id/Z4DrFhdlO6Xf13Xd@ip-10-97-1-34.eu-west-3.compute.internal
> 
> I could tweak that around the beginning of next week with a proposal
> of patch.  Bertrand, perhaps you'd prefer hack on this one?

Yeah, I had in mind to work on it (in the exact same line as you are describing
above). I'll work on it.

Regards,

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

  87. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2025-01-17T06:12:36Z

    On Fri, Jan 17, 2025 at 06:06:35AM +0000, Bertrand Drouvot wrote:
> On Fri, Jan 17, 2025 at 09:08:02AM +0900, Michael Paquier wrote:
>> I could tweak that around the beginning of next week with a proposal
>> of patch.  Bertrand, perhaps you'd prefer hack on this one?
> 
> Yeah, I had in mind to work on it (in the exact same line as you are describing
> above). I'll work on it.

Thanks.
--
Michael

  88. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-01-17T10:23:48Z

    Hi,

On Fri, Jan 17, 2025 at 03:12:36PM +0900, Michael Paquier wrote:
> On Fri, Jan 17, 2025 at 06:06:35AM +0000, Bertrand Drouvot wrote:
> > On Fri, Jan 17, 2025 at 09:08:02AM +0900, Michael Paquier wrote:
> >> I could tweak that around the beginning of next week with a proposal
> >> of patch.  Bertrand, perhaps you'd prefer hack on this one?
> > 
> > Yeah, I had in mind to work on it (in the exact same line as you are describing
> > above). I'll work on it.

Please find attached a patch implementing the ideas above, meaning:

- It creates a new PendingBackendStats variable
- It uses this variable to increment and flush per backend pending IO statistics

That way we get rid of the memory allocation for pending IO statistics.

One remark: a special case has been added in pgstat_flush_pending_entries(). The
reason is that while the per backend stats are "variable-numbered" stats, it could
be that their pending stats are not part of pgStatPending. This is currently the
case (with this patch) as the per backend pending IO stats are not tracked with
pgstat_prep_backend_pending() and friends anymore.

Thoughts?

Regards,

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

  89. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2025-01-18T08:53:31Z

    On Fri, Jan 17, 2025 at 10:23:48AM +0000, Bertrand Drouvot wrote:
> Please find attached a patch implementing the ideas above, meaning:
> 
> - It creates a new PendingBackendStats variable
> - It uses this variable to increment and flush per backend pending IO statistics
> 
> That way we get rid of the memory allocation for pending IO statistics.
> 
> One remark: a special case has been added in pgstat_flush_pending_entries(). The
> reason is that while the per backend stats are "variable-numbered" stats, it could
> be that their pending stats are not part of pgStatPending. This is currently the
> case (with this patch) as the per backend pending IO stats are not tracked with
> pgstat_prep_backend_pending() and friends anymore.


+    /*
+     * There is a special case for some pending stats that are tracked in
+     * PendingBackendStats. It's possible that those have not been flushed
+     * above, hence the extra check here.
+     */
+    if (!pg_memory_is_all_zeros(&PendingBackendStats,
+                                sizeof(struct PgStat_BackendPending)))
+    {
+        PgStat_EntryRef *entry_ref;
+
+        entry_ref = pgstat_get_entry_ref(PGSTAT_KIND_BACKEND, InvalidOid,
+                                         MyProcNumber, false, NULL);

Hmm.  Such special complexities in pgstat.c are annoying.  There is
a stupid thing I am wondering here.  For the WAL stats, why couldn't
we place some calls of pgstat_prep_backend_pending() in strategic
places like XLogBeginInsert() to force all the allocation steps of the
pending entry to happen before we would enter the critical sections
when doing a WAL insertion?  As far as I can see, there is a special
case with 2PC where XLogBeginInsert() could be called in a critical
section, but that seems to be the only one at quick glance.
--
Michael

  90. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2025-01-20T06:34:41Z

    On Sat, Jan 18, 2025 at 05:53:31PM +0900, Michael Paquier wrote:
> Hmm.  Such special complexities in pgstat.c are annoying.  There is
> a stupid thing I am wondering here.  For the WAL stats, why couldn't
> we place some calls of pgstat_prep_backend_pending() in strategic
> places like XLogBeginInsert() to force all the allocation steps of the
> pending entry to happen before we would enter the critical sections
> when doing a WAL insertion?  As far as I can see, there is a special
> case with 2PC where XLogBeginInsert() could be called in a critical
> section, but that seems to be the only one at quick glance.

I've looked first at this idea over the week-end with a quick hack,
and it did not finish well.

And then it struck me that with a bit of redesign of the callbacks, so
as the existing flush_fixed_cb and have_fixed_pending_cb are usable
with variable-numbered stats, we should be able to avoid the exception
your first version of the patch has introduced.  Attached is a patch
to achieve what I have in mind, which is more generic than your
previous patch.  I've built my stuff as 0002 on top of your 0001.

One key choice I have made is to hide PendingBackendStats within
pgstat_backend.c so as it is possible to enforce some sanity checks
there.
--
Michael

  91. Re: per backend I/O statistics

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

    Hi,

On Mon, Jan 20, 2025 at 03:34:41PM +0900, Michael Paquier wrote:
> On Sat, Jan 18, 2025 at 05:53:31PM +0900, Michael Paquier wrote:
> > Hmm.  Such special complexities in pgstat.c are annoying.  There is
> > a stupid thing I am wondering here.  For the WAL stats, why couldn't
> > we place some calls of pgstat_prep_backend_pending() in strategic
> > places like XLogBeginInsert() to force all the allocation steps of the
> > pending entry to happen before we would enter the critical sections
> > when doing a WAL insertion?  As far as I can see, there is a special
> > case with 2PC where XLogBeginInsert() could be called in a critical
> > section, but that seems to be the only one at quick glance.
> 
> I've looked first at this idea over the week-end with a quick hack,
> and it did not finish well.

Yeah, that would probably also mean moving the current WAL pending increments to
the same "new" place (to be consistent) which does not seem great.

> And then it struck me that with a bit of redesign of the callbacks, so

Thanks! 

> as the existing flush_fixed_cb and have_fixed_pending_cb are usable
> with variable-numbered stats, we should be able to avoid the exception
> your first version of the patch has introduced.  Attached is a patch
> to achieve what I have in mind, which is more generic than your
> previous patch.  I've built my stuff as 0002 on top of your 0001.
>

Indeed. Another idea that I had would have been to create another callback,
but I prefer your idea.

=== 1

I think that flush_pending_cb, flush_cb and have_pending_cb are now somehow
confusing (one could think that have_pending_cb is only linked to
flush_pending_cb).

I think that it would be better to make the distinction based on "local/static"
vs "dynamic memory" pending stats instead: I did so in v3 attached, using:

.flush_dynamic_cb(): flushes pending entries tracked in dynamic memory
.flush_static_cb(): flushes pending stats from static/global variable

Also I reworded the comments a bit.

=== 2

-       /*
-        * Clear out the statistics buffer, so it can be re-used.
-        */
-       MemSet(&PendingBackendStats.pending_io, 0, sizeof(PgStat_PendingIO));

The inital intent was to clear *only" the pending IO stats, so that each
flush (IO, WAL once implemented,...) could clear the pending stats it is
responsible for.

=== 3

+       /*
+        * Clear out the statistics buffer, so it can be re-used.
+        */
+       MemSet(&PendingBackendStats, 0, sizeof(PgStat_BackendPending));

Not sure about this one, see above. I mean it is currently Ok but once we'll
introduce the WAL part then it will not be correct depending of the flag value
being passed.

So, I did put back the previous logic in place (setting to zero only the stats
the flush callback is responsible for) in v3 attached.

=== 4

+ * Backend statistics counts waiting to be flushed out.  We assume this variable
+ * inits to zeroes.  These counters may be reported within critical sections so
+ * we use static memory in order to avoid memory allocation.
+ */
+static PgStat_BackendPending PendingBackendStats = {0};

I now think we should memset to zero to avoid any padding issues (as more structs
will be added to PgStat_BackendPending). Oh and it's already done in
pgstat_create_backend(), so removing the {0} assignement in the attached.

=== 5

+       have_backendstats = false;

I don't think setting have_backendstats to false in pgstat_flush_backend()
is correct. I mean, it is correct currently but once we'll add the WAL part it
won't necessary be correct if the flags != PGSTAT_BACKEND_FLUSH_ALL. So, using a 
pg_memory_is_all_zeros check on PendingBackendStats instead in the attached.

> One key choice I have made is to hide PendingBackendStats within
> pgstat_backend.c so as it is possible to enforce some sanity checks
> there.

Yeah, better, thanks!

=== 6

In passing, I realized that:

"
 * Copyright (c) 2001-2025, PostgreSQL Global Development Group
"

in pgstat_backend.c is incorrect (fixing it in 0002).

PFA:

0001: which is full rebase
0002: the Copyright fix
.txt: the changes I've made on top of your 0002 (to not confuse the cfbot and
to ease your review).

Regards,

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

  92. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2025-01-20T12:11:16Z

    On Mon, Jan 20, 2025 at 11:10:40AM +0000, Bertrand Drouvot wrote:
> I think that it would be better to make the distinction based on "local/static"
> vs "dynamic memory" pending stats instead: I did so in v3 attached, using:
> 
> .flush_dynamic_cb(): flushes pending entries tracked in dynamic memory
> .flush_static_cb(): flushes pending stats from static/global variable

"static" and "dynamic" feel a bit off when used together.  For
"static", the stats are from a static state but they can be pushed to
the dshash, as well, which is in dynamic shared memory.

Hmm.  Thinking more about that, I think that we should leave the
existing "flush_pending_cb" alone.  For the two others, how about
"have_static_pending_cb" and "flush_static_cb" rather than "fixed"?
It seems important to me to show the relationship between these two.

> Not sure about this one, see above. I mean it is currently Ok but once we'll
> introduce the WAL part then it will not be correct depending of the flag value
> being passed.
> 
> So, I did put back the previous logic in place (setting to zero only the stats
> the flush callback is responsible for) in v3 attached.

Hmm.  I think that I'm OK with what you are doing here with the next
parts in mind, based on this argument.

> I don't think setting have_backendstats to false in pgstat_flush_backend()
> is correct. I mean, it is correct currently but once we'll add the WAL part it
> won't necessary be correct if the flags != PGSTAT_BACKEND_FLUSH_ALL. So, using a 
> pg_memory_is_all_zeros check on PendingBackendStats instead in the attached.

Indeed.  I got this part slightly wrong here.  What you are doing with
an all-zero check looks simpler in the long run.
--
Michael

  93. Re: per backend I/O statistics

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-01-20T13:26:55Z

    Hi,

On Mon, Jan 20, 2025 at 09:11:16PM +0900, Michael Paquier wrote:
> On Mon, Jan 20, 2025 at 11:10:40AM +0000, Bertrand Drouvot wrote:
> > I think that it would be better to make the distinction based on "local/static"
> > vs "dynamic memory" pending stats instead: I did so in v3 attached, using:
> > 
> > .flush_dynamic_cb(): flushes pending entries tracked in dynamic memory
> > .flush_static_cb(): flushes pending stats from static/global variable
> 
> "static" and "dynamic" feel a bit off when used together.  For
> "static", the stats are from a static state but they can be pushed to
> the dshash, as well, which is in dynamic shared memory.

Right, that's another way to find it confusing ;-). But if we keep focus
on where the pending stats are stored, it is not. 

> Hmm.  Thinking more about that, I think that we should leave the
> existing "flush_pending_cb" alone.  For the two others, how about
> "have_static_pending_cb" and "flush_static_cb" rather than "fixed"?
> It seems important to me to show the relationship between these two.

Yeah, that works for me: done in v4 attached.

Regards,

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

  94. Re: per backend I/O statistics

    Michael Paquier <michael@paquier.xyz> — 2025-01-21T02:56:12Z

    On Mon, Jan 20, 2025 at 01:26:55PM +0000, Bertrand Drouvot wrote:
> Right, that's another way to find it confusing ;-). But if we keep focus
> on where the pending stats are stored, it is not.

The name of the callbacks can be tuned infinitely.  I have split the
renaming into its own patch, then the result looked fine after a
second look so I have applied both things separately to unlock all the
WAL patches.
--
Michael