Thread: Introduce a new view for checkpointer related stats

Hi,

pg_stat_bgwriter view currently reports checkpointer stats as well. It
is that way because historically checkpointer was part of bgwriter
until the commits 806a2ae and bf405ba, that went into PG 9.2,
separated them out. I think it is time for us to separate checkpointer
stats to its own view. I discussed it in another thread [1] and it
seems like there's an unequivocal agreement for the proposal.

I'm attaching a patch introducing a new pg_stat_checkpointer view,
with this change the pg_stat_bgwriter view only focuses on bgwriter
related stats. The patch does mostly mechanical changes. I'll add it
to CF in a bit.

Thoughts?

[1]

https://www.postgresql.org/message-id/flat/20221116181433.y2hq2pirtbqmmndt%40awork3.anarazel.de#b873a4bd7d8d7ec70750a7047db33f56
https://www.postgresql.org/message-id/CA%2BTgmoYCu6RpuJ3cZz0e7cZSfaVb%3Dcr6iVcgGMGd5dxX0MYNRA%40mail.gmail.com

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

Hi,

On 11/17/22 1:51 PM, Bharath Rupireddy wrote:
> Hi,
> 
> pg_stat_bgwriter view currently reports checkpointer stats as well. It
> is that way because historically checkpointer was part of bgwriter
> until the commits 806a2ae and bf405ba, that went into PG 9.2,
> separated them out. I think it is time for us to separate checkpointer
> stats to its own view. I discussed it in another thread [1] and it
> seems like there's an unequivocal agreement for the proposal.
> 
> I'm attaching a patch introducing a new pg_stat_checkpointer view,
> with this change the pg_stat_bgwriter view only focuses on bgwriter
> related stats. The patch does mostly mechanical changes. I'll add it
> to CF in a bit.
> 
> Thoughts?

+1 for the dedicated view.

+  <para>
+   The <structname>pg_stat_checkpointer</structname> view will always have a
+   single row, containing global data for the cluster.
+  </para>

what about "containing data about checkpointer activity of the cluster"? (to provide more "details" (even if that seems
obviousgiven the name of the view) and be consistent with the pg_stat_wal description too).
 
And if it makes sense to you, While at it, why not go for "containing data about bgwriter activity of the cluster" for
pg_stat_bgwritertoo?
 

+CREATE VIEW pg_stat_checkpointer AS
+    SELECT
+        pg_stat_get_timed_checkpoints() AS checkpoints_timed,
+        pg_stat_get_requested_checkpoints() AS checkpoints_req,
+        pg_stat_get_checkpoint_write_time() AS checkpoint_write_time,
+        pg_stat_get_checkpoint_sync_time() AS checkpoint_sync_time,
+        pg_stat_get_buf_written_checkpoints() AS buffers_checkpoint,
+        pg_stat_get_buf_written_backend() AS buffers_backend,
+        pg_stat_get_buf_fsync_backend() AS buffers_backend_fsync,
+        pg_stat_get_checkpointer_stat_reset_time() AS stats_reset;

I don't think we should keep the checkpoints_ prefix (or _checkpoint suffix) in the column names now that they belong
toa dedicated view (also the pg_stat_bgwriter view's columns don't have a
 
bgwriter prefix/suffix).

And while at it, I'm not sure the wal_ suffix in pg_stat_wal make sense too.

The idea is to have consistent naming between the views and their columns: I'd vote without prefix/suffix.

Regards,

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

On Tue, Nov 22, 2022 at 1:26 PM Drouvot, Bertrand
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On 11/17/22 1:51 PM, Bharath Rupireddy wrote:
> > Hi,
> >
> > pg_stat_bgwriter view currently reports checkpointer stats as well. It
> > is that way because historically checkpointer was part of bgwriter
> > until the commits 806a2ae and bf405ba, that went into PG 9.2,
> > separated them out. I think it is time for us to separate checkpointer
> > stats to its own view. I discussed it in another thread [1] and it
> > seems like there's an unequivocal agreement for the proposal.
> >
> > I'm attaching a patch introducing a new pg_stat_checkpointer view,
> > with this change the pg_stat_bgwriter view only focuses on bgwriter
> > related stats. The patch does mostly mechanical changes. I'll add it
> > to CF in a bit.
> >
> > Thoughts?
>
> +1 for the dedicated view.
>
> +  <para>
> +   The <structname>pg_stat_checkpointer</structname> view will always have a
> +   single row, containing global data for the cluster.
> +  </para>
>
> what about "containing data about checkpointer activity of the cluster"? (to provide more "details" (even if that
seemsobvious given the name of the view) and be consistent with the pg_stat_wal description too).
 
> And if it makes sense to you, While at it, why not go for "containing data about bgwriter activity of the cluster"
forpg_stat_bgwriter too?
 

Nice catch. Modified.

> +CREATE VIEW pg_stat_checkpointer AS
> +    SELECT
> +        pg_stat_get_timed_checkpoints() AS checkpoints_timed,
> +        pg_stat_get_requested_checkpoints() AS checkpoints_req,
> +        pg_stat_get_checkpoint_write_time() AS checkpoint_write_time,
> +        pg_stat_get_checkpoint_sync_time() AS checkpoint_sync_time,
> +        pg_stat_get_buf_written_checkpoints() AS buffers_checkpoint,
> +        pg_stat_get_buf_written_backend() AS buffers_backend,
> +        pg_stat_get_buf_fsync_backend() AS buffers_backend_fsync,
> +        pg_stat_get_checkpointer_stat_reset_time() AS stats_reset;
>
> I don't think we should keep the checkpoints_ prefix (or _checkpoint suffix) in the column names now that they belong
toa dedicated view (also the pg_stat_bgwriter view's columns don't have a
 
> bgwriter prefix/suffix).
>
> And while at it, I'm not sure the wal_ suffix in pg_stat_wal make sense too.
>
> The idea is to have consistent naming between the views and their columns: I'd vote without prefix/suffix.

-1. If the prefix is removed, some column names become unreadable -
timed, requested, write_time, sync_time, buffers. We might think of
renaming those columns to something more readable, I tend to not do
that as it can break largely the application/service layer/monitoring
tools, of course even with the new pg_stat_checkpointer view, we can't
avoid that, however the changes are less i.e. replace pg_stat_bgwriter
with the new view.

I'm attaching the v2 patch for further review.

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

Hi,

On 2022-11-22 18:08:28 +0530, Bharath Rupireddy wrote:
> diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
> index 2d8104b090..131d949dfb 100644
> --- a/src/backend/catalog/system_views.sql
> +++ b/src/backend/catalog/system_views.sql
> @@ -1105,18 +1105,22 @@ CREATE VIEW pg_stat_archiver AS
>  
>  CREATE VIEW pg_stat_bgwriter AS
>      SELECT
> -        pg_stat_get_bgwriter_timed_checkpoints() AS checkpoints_timed,
> -        pg_stat_get_bgwriter_requested_checkpoints() AS checkpoints_req,
> -        pg_stat_get_checkpoint_write_time() AS checkpoint_write_time,
> -        pg_stat_get_checkpoint_sync_time() AS checkpoint_sync_time,
> -        pg_stat_get_bgwriter_buf_written_checkpoints() AS buffers_checkpoint,
>          pg_stat_get_bgwriter_buf_written_clean() AS buffers_clean,
>          pg_stat_get_bgwriter_maxwritten_clean() AS maxwritten_clean,
> -        pg_stat_get_buf_written_backend() AS buffers_backend,
> -        pg_stat_get_buf_fsync_backend() AS buffers_backend_fsync,
>          pg_stat_get_buf_alloc() AS buffers_alloc,
>          pg_stat_get_bgwriter_stat_reset_time() AS stats_reset;
>  
> +CREATE VIEW pg_stat_checkpointer AS
> +    SELECT
> +        pg_stat_get_timed_checkpoints() AS checkpoints_timed,
> +        pg_stat_get_requested_checkpoints() AS checkpoints_req,
> +        pg_stat_get_checkpoint_write_time() AS checkpoint_write_time,
> +        pg_stat_get_checkpoint_sync_time() AS checkpoint_sync_time,
> +        pg_stat_get_buf_written_checkpoints() AS buffers_checkpoint,
> +        pg_stat_get_buf_written_backend() AS buffers_backend,
> +        pg_stat_get_buf_fsync_backend() AS buffers_backend_fsync,
> +        pg_stat_get_checkpointer_stat_reset_time() AS stats_reset;


I think we should consider deprecating the pg_stat_bgwriter columns but
leaving them in place for a few years. New stuff should only be added to
pg_stat_checkpointer, but we don't need to break old monitoring queries.

Greetings,

Andres Freund

On Wed, Nov 23, 2022 at 2:23 AM Andres Freund <andres@anarazel.de> wrote:
>
> On 2022-11-22 18:08:28 +0530, Bharath Rupireddy wrote:
> >
> >  CREATE VIEW pg_stat_bgwriter AS
> >      SELECT
> > -        pg_stat_get_bgwriter_timed_checkpoints() AS checkpoints_timed,
> > -        pg_stat_get_bgwriter_requested_checkpoints() AS checkpoints_req,
> > -        pg_stat_get_checkpoint_write_time() AS checkpoint_write_time,
> > -        pg_stat_get_checkpoint_sync_time() AS checkpoint_sync_time,
> > -        pg_stat_get_bgwriter_buf_written_checkpoints() AS buffers_checkpoint,
> >          pg_stat_get_bgwriter_buf_written_clean() AS buffers_clean,
> >          pg_stat_get_bgwriter_maxwritten_clean() AS maxwritten_clean,
> > -        pg_stat_get_buf_written_backend() AS buffers_backend,
> > -        pg_stat_get_buf_fsync_backend() AS buffers_backend_fsync,
> >          pg_stat_get_buf_alloc() AS buffers_alloc,
> >          pg_stat_get_bgwriter_stat_reset_time() AS stats_reset;
>
>
> I think we should consider deprecating the pg_stat_bgwriter columns but
> leaving them in place for a few years. New stuff should only be added to
> pg_stat_checkpointer, but we don't need to break old monitoring queries.

May I know what it means to deprecate pg_stat_bgwriter columns? Are
you suggesting to add deprecation warnings to corresponding functions
pg_stat_get_bgwriter_buf_written_clean(),
pg_stat_get_bgwriter_maxwritten_clean(), pg_stat_get_buf_alloc() and
pg_stat_get_bgwriter_stat_reset_time() and in the docs? And eventually
do away with the bgwriter stats and the file pgstat_bgwriter.c? Aren't
the bgwriter stats buf_written_clean, maxwritten_clean  and buf_alloc
useful?

I think we need to discuss the pg_stat_bgwriter deprecation separately
independent of the patch here, no?

PS: I noticed some discussion here
https://www.postgresql.org/message-id/20221121003815.qnwlnz2lhkow2e5w%40awork3.anarazel.de,
I haven't spent enough time on it.

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

Hi,

On 2022-11-23 11:39:43 +0530, Bharath Rupireddy wrote:
> On Wed, Nov 23, 2022 at 2:23 AM Andres Freund <andres@anarazel.de> wrote:
> >
> > On 2022-11-22 18:08:28 +0530, Bharath Rupireddy wrote:
> > >
> > >  CREATE VIEW pg_stat_bgwriter AS
> > >      SELECT
> > > -        pg_stat_get_bgwriter_timed_checkpoints() AS checkpoints_timed,
> > > -        pg_stat_get_bgwriter_requested_checkpoints() AS checkpoints_req,
> > > -        pg_stat_get_checkpoint_write_time() AS checkpoint_write_time,
> > > -        pg_stat_get_checkpoint_sync_time() AS checkpoint_sync_time,
> > > -        pg_stat_get_bgwriter_buf_written_checkpoints() AS buffers_checkpoint,
> > >          pg_stat_get_bgwriter_buf_written_clean() AS buffers_clean,
> > >          pg_stat_get_bgwriter_maxwritten_clean() AS maxwritten_clean,
> > > -        pg_stat_get_buf_written_backend() AS buffers_backend,
> > > -        pg_stat_get_buf_fsync_backend() AS buffers_backend_fsync,
> > >          pg_stat_get_buf_alloc() AS buffers_alloc,
> > >          pg_stat_get_bgwriter_stat_reset_time() AS stats_reset;
> >
> >
> > I think we should consider deprecating the pg_stat_bgwriter columns but
> > leaving them in place for a few years. New stuff should only be added to
> > pg_stat_checkpointer, but we don't need to break old monitoring queries.
> 
> May I know what it means to deprecate pg_stat_bgwriter columns?

Add a note to the docs saying that the columns will be removed.


> Are
> you suggesting to add deprecation warnings to corresponding functions
> pg_stat_get_bgwriter_buf_written_clean(),
> pg_stat_get_bgwriter_maxwritten_clean(), pg_stat_get_buf_alloc() and
> pg_stat_get_bgwriter_stat_reset_time() and in the docs?

I'm thinking of the checkpoint related columns in pg_stat_bgwriter.

If we move, rather than duplicate, the pg_stat_bgwriter columns to
pg_stat_checkpointer, everyone will have to update their monitoring scripts
when upgrading and will need to add version dependency if they monitor
multiple versions. If we instead keep the duplicated columns in
pg_stat_bgwriter for 5 years, users can reloy on pg_stat_checkpointer in all
supported versions.

To be clear, it isn't a very heavy burden for users to make these
adjustments. But if it only costs us a few lines to keep the old columns for a
bit, that seems worth it.


> And eventually do away with the bgwriter stats and the file
> pgstat_bgwriter.c? Aren't the bgwriter stats buf_written_clean,
> maxwritten_clean and buf_alloc useful?

Correct, I don't think we should remove those.

Greetings,

Andres Freund

On Sat, Nov 26, 2022 at 4:32 AM Andres Freund <andres@anarazel.de> wrote:
>

Thanks Andres for reviewing.

> > May I know what it means to deprecate pg_stat_bgwriter columns?
>
> Add a note to the docs saying that the columns will be removed.

Done.

> > Are
> > you suggesting to add deprecation warnings to corresponding functions
> > pg_stat_get_bgwriter_buf_written_clean(),
> > pg_stat_get_bgwriter_maxwritten_clean(), pg_stat_get_buf_alloc() and
> > pg_stat_get_bgwriter_stat_reset_time() and in the docs?
>
> I'm thinking of the checkpoint related columns in pg_stat_bgwriter.

Added note in the docs alongside each deprecated pg_stat_bgwriter's
checkpoint related column.

> If we move, rather than duplicate, the pg_stat_bgwriter columns to
> pg_stat_checkpointer, everyone will have to update their monitoring scripts
> when upgrading and will need to add version dependency if they monitor
> multiple versions. If we instead keep the duplicated columns in
> pg_stat_bgwriter for 5 years, users can reloy on pg_stat_checkpointer in all
> supported versions.

Agree. However, it's a bit difficult to keep track of deprecated
things and come back after 5 years to clean them up unless "some"
postgres hacker/developer/user notices it again. Perhaps, adding a
separate section, say 'Deprecated and To-Be-Removed, in
https://wiki.postgresql.org/wiki/Main_Page is a good idea.

> To be clear, it isn't a very heavy burden for users to make these
> adjustments. But if it only costs us a few lines to keep the old columns for a
> bit, that seems worth it.

Yes.

I'm attaching the v3 patch for further review.

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

On Tue, Nov 22, 2022 at 3:53 PM Andres Freund <andres@anarazel.de> wrote:
> I think we should consider deprecating the pg_stat_bgwriter columns but
> leaving them in place for a few years. New stuff should only be added to
> pg_stat_checkpointer, but we don't need to break old monitoring queries.

I vote to just remove them. I think that most people won't update
their queries until they are forced to do so.  I don't think it
matters very much when we force them to do that.

Our track record in following through on deprecations is pretty bad
too, which is another consideration.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, Nov 28, 2022 at 11:29 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Tue, Nov 22, 2022 at 3:53 PM Andres Freund <andres@anarazel.de> wrote:
> > I think we should consider deprecating the pg_stat_bgwriter columns but
> > leaving them in place for a few years. New stuff should only be added to
> > pg_stat_checkpointer, but we don't need to break old monitoring queries.
>
> I vote to just remove them. I think that most people won't update
> their queries until they are forced to do so.  I don't think it
> matters very much when we force them to do that.
>
> Our track record in following through on deprecations is pretty bad
> too, which is another consideration.

Hm. I'm fine with either way. Even if we remove the checkpointer
columns from pg_stat_bgwriter, the changes that one needs to do are so
minimal and straightforward because the column names aren't changed,
just the view name.

Having said that, I don't have a strong opinion here. I'll leave it to
the other hacker's opinion and/or committer's discretion.

FWIW - here's the v2 patch that gets rid of checkpointer columns from
pg_stat_bgwriter
https://www.postgresql.org/message-id/CALj2ACX8jFET1C3bs_edz_8JRcMg5nz8Y7ryjGaCsfnVpAYoVQ%40mail.gmail.com
and here's the v3 patch that deprecates
https://www.postgresql.org/message-id/CALj2ACUjEvPQYGJHmH2FrAj1gmvHskNrOeNUr7xnwjtkYVZvEQ%40mail.gmail.com.

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

On Mon, Nov 28, 2022 at 11:29 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Tue, Nov 22, 2022 at 3:53 PM Andres Freund <andres@anarazel.de> wrote:
> > I think we should consider deprecating the pg_stat_bgwriter columns but
> > leaving them in place for a few years. New stuff should only be added to
> > pg_stat_checkpointer, but we don't need to break old monitoring queries.
>
> I vote to just remove them. I think that most people won't update
> their queries until they are forced to do so.  I don't think it
> matters very much when we force them to do that.
>

+1.

-- 
With Regards,
Amit Kapila.

Hi,

On 11/28/22 6:58 PM, Robert Haas wrote:
> On Tue, Nov 22, 2022 at 3:53 PM Andres Freund <andres@anarazel.de> wrote:
>> I think we should consider deprecating the pg_stat_bgwriter columns but
>> leaving them in place for a few years. New stuff should only be added to
>> pg_stat_checkpointer, but we don't need to break old monitoring queries.
> 
> I vote to just remove them. I think that most people won't update
> their queries until they are forced to do so.  I don't think it
> matters very much when we force them to do that.
> 
> Our track record in following through on deprecations is pretty bad
> too, which is another consideration.
> 

Same point of view.

Regards,

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

On Mon, 28 Nov 2022 at 13:00, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Tue, Nov 22, 2022 at 3:53 PM Andres Freund <andres@anarazel.de> wrote:

> I vote to just remove them. I think that most people won't update
> their queries until they are forced to do so.  I don't think it
> matters very much when we force them to do that.

I would tend to agree.

If we wanted to have a deprecation period I think the smooth way to do
it would be to introduce two new functions/views with the new split.
Then mark the entire old view as deprecated. That way there isn't a
mix of new and old columns in the same view/function.

I don't think it's really necessary to do that here but there'll
probably be instances where it would be worth doing.

-- 
greg

Hi,

On 2022-11-28 12:58:48 -0500, Robert Haas wrote:
> On Tue, Nov 22, 2022 at 3:53 PM Andres Freund <andres@anarazel.de> wrote:
> > I think we should consider deprecating the pg_stat_bgwriter columns but
> > leaving them in place for a few years. New stuff should only be added to
> > pg_stat_checkpointer, but we don't need to break old monitoring queries.
> 
> I vote to just remove them. I think that most people won't update
> their queries until they are forced to do so.

Seems most agree with that... WFM.

But:


> I don't think it matters very much when we force them to do that.

I don't think that's true. If we remove the columns when the last version
without pg_stat_checkpointer has gone out of support, users don't need to have
version switches in their monitoring setups.

Greetings,

Andres Freund

On Wed, Nov 30, 2022 at 6:01 AM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2022-11-28 12:58:48 -0500, Robert Haas wrote:
> > On Tue, Nov 22, 2022 at 3:53 PM Andres Freund <andres@anarazel.de> wrote:
> > > I think we should consider deprecating the pg_stat_bgwriter columns but
> > > leaving them in place for a few years. New stuff should only be added to
> > > pg_stat_checkpointer, but we don't need to break old monitoring queries.
> >
> > I vote to just remove them. I think that most people won't update
> > their queries until they are forced to do so.
>
> Seems most agree with that... WFM.

Thanks. I'm attaching the v2 patch from upthread again here as we all
agree to remove checkpointer columns from pg_stat_bgwriter view and
have them in the new view pg_stat_checkpointer.

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

Hi,

On 11/30/22 7:34 AM, Bharath Rupireddy wrote:
> On Wed, Nov 30, 2022 at 6:01 AM Andres Freund <andres@anarazel.de> wrote:
>>
>> Hi,
>>
>> On 2022-11-28 12:58:48 -0500, Robert Haas wrote:
>>> On Tue, Nov 22, 2022 at 3:53 PM Andres Freund <andres@anarazel.de> wrote:
>>>> I think we should consider deprecating the pg_stat_bgwriter columns but
>>>> leaving them in place for a few years. New stuff should only be added to
>>>> pg_stat_checkpointer, but we don't need to break old monitoring queries.
>>>
>>> I vote to just remove them. I think that most people won't update
>>> their queries until they are forced to do so.
>>
>> Seems most agree with that... WFM.
> 
> Thanks. I'm attaching the v2 patch from upthread again here as we all
> agree to remove checkpointer columns from pg_stat_bgwriter view and
> have them in the new view pg_stat_checkpointer.
> 

+CREATE VIEW pg_stat_checkpointer AS
+    SELECT
+        pg_stat_get_timed_checkpoints() AS checkpoints_timed,
+        pg_stat_get_requested_checkpoints() AS checkpoints_req,
+        pg_stat_get_checkpoint_write_time() AS checkpoint_write_time,
+        pg_stat_get_checkpoint_sync_time() AS checkpoint_sync_time,
+        pg_stat_get_buf_written_checkpoints() AS buffers_checkpoint,
+        pg_stat_get_buf_written_backend() AS buffers_backend,
+        pg_stat_get_buf_fsync_backend() AS buffers_backend_fsync,
+        pg_stat_get_checkpointer_stat_reset_time() AS stats_reset;
+

I still think that having checkpoints_ prefix in a pg_stat_checkpointer view sounds "weird" (made sense when they were
partof pg_stat_bgwriter)
 

maybe we could have something like this instead?

+CREATE VIEW pg_stat_checkpointer AS
+    SELECT
+        pg_stat_get_timed_checkpoints() AS num_timed,
+        pg_stat_get_requested_checkpoints() AS num_req,
+        pg_stat_get_checkpoint_write_time() AS total_write_time,
+        pg_stat_get_checkpoint_sync_time() AS total_sync_time,
+        pg_stat_get_buf_written_checkpoints() AS buffers_checkpoint,
+        pg_stat_get_buf_written_backend() AS buffers_backend,
+        pg_stat_get_buf_fsync_backend() AS buffers_backend_fsync,
+        pg_stat_get_checkpointer_stat_reset_time() AS stats_reset;
+

That's a nit in any case and the patch LGTM.

Regards,

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

On Wed, Nov 30, 2022 at 12:44 PM Drouvot, Bertrand
<bertranddrouvot.pg@gmail.com> wrote:
>
> +CREATE VIEW pg_stat_checkpointer AS
> +    SELECT
> +        pg_stat_get_timed_checkpoints() AS checkpoints_timed,
> +        pg_stat_get_requested_checkpoints() AS checkpoints_req,
> +        pg_stat_get_checkpoint_write_time() AS checkpoint_write_time,
> +        pg_stat_get_checkpoint_sync_time() AS checkpoint_sync_time,
> +        pg_stat_get_buf_written_checkpoints() AS buffers_checkpoint,
> +        pg_stat_get_buf_written_backend() AS buffers_backend,
> +        pg_stat_get_buf_fsync_backend() AS buffers_backend_fsync,
> +        pg_stat_get_checkpointer_stat_reset_time() AS stats_reset;
> +
>
> I still think that having checkpoints_ prefix in a pg_stat_checkpointer view sounds "weird" (made sense when they
werepart of pg_stat_bgwriter)
 
>
> maybe we could have something like this instead?
>
> +CREATE VIEW pg_stat_checkpointer AS
> +    SELECT
> +        pg_stat_get_timed_checkpoints() AS num_timed,
> +        pg_stat_get_requested_checkpoints() AS num_req,
> +        pg_stat_get_checkpoint_write_time() AS total_write_time,
> +        pg_stat_get_checkpoint_sync_time() AS total_sync_time,
> +        pg_stat_get_buf_written_checkpoints() AS buffers_checkpoint,
> +        pg_stat_get_buf_written_backend() AS buffers_backend,
> +        pg_stat_get_buf_fsync_backend() AS buffers_backend_fsync,
> +        pg_stat_get_checkpointer_stat_reset_time() AS stats_reset;
> +

I don't have a strong opinion about changing column names. However, if
we were to change it, I prefer to use names that
PgStat_CheckpointerStats has. BTW, that's what
PgStat_BgWriterStats/pg_stat_bgwriter and
PgStat_ArchiverStats/pg_stat_archiver uses.
typedef struct PgStat_CheckpointerStats
{
    PgStat_Counter timed_checkpoints;
    PgStat_Counter requested_checkpoints;
    PgStat_Counter checkpoint_write_time;   /* times in milliseconds */
    PgStat_Counter checkpoint_sync_time;
    PgStat_Counter buf_written_checkpoints;
    PgStat_Counter buf_written_backend;
    PgStat_Counter buf_fsync_backend;
    TimestampTz stat_reset_timestamp;
} PgStat_CheckpointerStats;

> That's a nit in any case and the patch LGTM.

Thanks.

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

On Wed, Nov 30, 2022 at 5:15 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> I don't have a strong opinion about changing column names. However, if
> we were to change it, I prefer to use names that
> PgStat_CheckpointerStats has. BTW, that's what
> PgStat_BgWriterStats/pg_stat_bgwriter and
> PgStat_ArchiverStats/pg_stat_archiver uses.

After thinking about this a while, I convinced myself to change the
column names to be a bit more meaningful. I still think having
checkpoints in the column names is needed because it also has other
backend related columns. I'm attaching the v4 patch for further
review.
CREATE VIEW pg_stat_checkpointer AS
    SELECT
        pg_stat_get_timed_checkpoints() AS timed_checkpoints,
        pg_stat_get_requested_checkpoints() AS requested_checkpoints,
        pg_stat_get_checkpoint_write_time() AS checkpoint_write_time,
        pg_stat_get_checkpoint_sync_time() AS checkpoint_sync_time,
        pg_stat_get_buf_written_checkpoints() AS buffers_written_checkpoints,
        pg_stat_get_buf_written_backend() AS buffers_written_backend,
        pg_stat_get_buf_fsync_backend() AS buffers_fsync_backend,
        pg_stat_get_checkpointer_stat_reset_time() AS stats_reset;

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

On Fri, Dec 2, 2022 at 12:54 PM sirisha chamarthi
<sirichamarthi22@gmail.com> wrote:
>
> On Thu, Dec 1, 2022 at 9:50 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
>>
>> On Wed, Nov 30, 2022 at 5:15 PM Bharath Rupireddy
>> <bharath.rupireddyforpostgres@gmail.com> wrote:
>> >
>> > I don't have a strong opinion about changing column names. However, if
>> > we were to change it, I prefer to use names that
>> > PgStat_CheckpointerStats has. BTW, that's what
>> > PgStat_BgWriterStats/pg_stat_bgwriter and
>> > PgStat_ArchiverStats/pg_stat_archiver uses.
>>
>> After thinking about this a while, I convinced myself to change the
>> column names to be a bit more meaningful. I still think having
>> checkpoints in the column names is needed because it also has other
>> backend related columns. I'm attaching the v4 patch for further
>> review.
>> CREATE VIEW pg_stat_checkpointer AS
>>     SELECT
>>         pg_stat_get_timed_checkpoints() AS timed_checkpoints,
>>         pg_stat_get_requested_checkpoints() AS requested_checkpoints,
>>         pg_stat_get_checkpoint_write_time() AS checkpoint_write_time,
>>         pg_stat_get_checkpoint_sync_time() AS checkpoint_sync_time,
>>         pg_stat_get_buf_written_checkpoints() AS buffers_written_checkpoints,
>>         pg_stat_get_buf_written_backend() AS buffers_written_backend,
>>         pg_stat_get_buf_fsync_backend() AS buffers_fsync_backend,
>>         pg_stat_get_checkpointer_stat_reset_time() AS stats_reset;
>
>
> IMO, “buffers_written_checkpoints” is confusing. What do you think?

Thanks. We can be "more and more" meaningful by naming
buffers_written_by_checkpoints, buffers_written_by_backend,
buffers_fsync_by_backend. However, I don't think that's a good idea
here as names get too long.

Having said that, I'll leave it to the committer's discretion.

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

Hi,

On 12/2/22 6:50 AM, Bharath Rupireddy wrote:
> On Wed, Nov 30, 2022 at 5:15 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
>>
>> I don't have a strong opinion about changing column names. However, if
>> we were to change it, I prefer to use names that
>> PgStat_CheckpointerStats has. BTW, that's what
>> PgStat_BgWriterStats/pg_stat_bgwriter and
>> PgStat_ArchiverStats/pg_stat_archiver uses.
> 
> After thinking about this a while, I convinced myself to change the
> column names to be a bit more meaningful. I still think having
> checkpoints in the column names is needed because it also has other
> backend related columns. I'm attaching the v4 patch for further
> review.
> CREATE VIEW pg_stat_checkpointer AS
>      SELECT
>          pg_stat_get_timed_checkpoints() AS timed_checkpoints,
>          pg_stat_get_requested_checkpoints() AS requested_checkpoints,
>          pg_stat_get_checkpoint_write_time() AS checkpoint_write_time,
>          pg_stat_get_checkpoint_sync_time() AS checkpoint_sync_time,
>          pg_stat_get_buf_written_checkpoints() AS buffers_written_checkpoints,
>          pg_stat_get_buf_written_backend() AS buffers_written_backend,
>          pg_stat_get_buf_fsync_backend() AS buffers_fsync_backend,
>          pg_stat_get_checkpointer_stat_reset_time() AS stats_reset;
> 

Thanks!

Patch LGTM, marking it as Ready for Committer.

Regards,

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

The patch looks good to me.

Thanks & Regards,
Nitin Jadhav

On Fri, Dec 2, 2022 at 11:20 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, Nov 30, 2022 at 5:15 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > I don't have a strong opinion about changing column names. However, if
> > we were to change it, I prefer to use names that
> > PgStat_CheckpointerStats has. BTW, that's what
> > PgStat_BgWriterStats/pg_stat_bgwriter and
> > PgStat_ArchiverStats/pg_stat_archiver uses.
>
> After thinking about this a while, I convinced myself to change the
> column names to be a bit more meaningful. I still think having
> checkpoints in the column names is needed because it also has other
> backend related columns. I'm attaching the v4 patch for further
> review.
> CREATE VIEW pg_stat_checkpointer AS
>     SELECT
>         pg_stat_get_timed_checkpoints() AS timed_checkpoints,
>         pg_stat_get_requested_checkpoints() AS requested_checkpoints,
>         pg_stat_get_checkpoint_write_time() AS checkpoint_write_time,
>         pg_stat_get_checkpoint_sync_time() AS checkpoint_sync_time,
>         pg_stat_get_buf_written_checkpoints() AS buffers_written_checkpoints,
>         pg_stat_get_buf_written_backend() AS buffers_written_backend,
>         pg_stat_get_buf_fsync_backend() AS buffers_fsync_backend,
>         pg_stat_get_checkpointer_stat_reset_time() AS stats_reset;
>
> --
> Bharath Rupireddy
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com

On Fri, Dec 02, 2022 at 08:36:38AM +0100, Drouvot, Bertrand wrote:
> Patch LGTM, marking it as Ready for Committer.

Unfortunately, this patch no longer applies.  Bharath, would you mind
posting a rebased version?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Fri, Dec 2, 2022 at 1:07 PM Drouvot, Bertrand
<bertranddrouvot.pg@gmail.com> wrote:
>
> Patch LGTM, marking it as Ready for Committer.

Had to rebase, attached v5 patch for further consideration.

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

On Sat, Jan 21, 2023 at 5:56 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Fri, Dec 2, 2022 at 1:07 PM Drouvot, Bertrand
> <bertranddrouvot.pg@gmail.com> wrote:
> >
> > Patch LGTM, marking it as Ready for Committer.
>
> Had to rebase, attached v5 patch for further consideration.

One more rebase due to 28e626bd (pgstat: Infrastructure for more
detailed IO statistics). PSA v6 patch.

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

Hi,

On 2023-02-09 12:21:51 +0530, Bharath Rupireddy wrote:
> @@ -1105,18 +1105,22 @@ CREATE VIEW pg_stat_archiver AS
>  
>  CREATE VIEW pg_stat_bgwriter AS
>      SELECT
> -        pg_stat_get_bgwriter_timed_checkpoints() AS checkpoints_timed,
> -        pg_stat_get_bgwriter_requested_checkpoints() AS checkpoints_req,
> -        pg_stat_get_checkpoint_write_time() AS checkpoint_write_time,
> -        pg_stat_get_checkpoint_sync_time() AS checkpoint_sync_time,
> -        pg_stat_get_bgwriter_buf_written_checkpoints() AS buffers_checkpoint,
>          pg_stat_get_bgwriter_buf_written_clean() AS buffers_clean,
>          pg_stat_get_bgwriter_maxwritten_clean() AS maxwritten_clean,
> -        pg_stat_get_buf_written_backend() AS buffers_backend,
> -        pg_stat_get_buf_fsync_backend() AS buffers_backend_fsync,
>          pg_stat_get_buf_alloc() AS buffers_alloc,
>          pg_stat_get_bgwriter_stat_reset_time() AS stats_reset;
>  
> +CREATE VIEW pg_stat_checkpointer AS
> +    SELECT
> +        pg_stat_get_timed_checkpoints() AS timed_checkpoints,
> +        pg_stat_get_requested_checkpoints() AS requested_checkpoints,
> +        pg_stat_get_checkpoint_write_time() AS checkpoint_write_time,
> +        pg_stat_get_checkpoint_sync_time() AS checkpoint_sync_time,
> +        pg_stat_get_buf_written_checkpoints() AS buffers_written_checkpoints,
> +        pg_stat_get_buf_written_backend() AS buffers_written_backend,
> +        pg_stat_get_buf_fsync_backend() AS buffers_fsync_backend,
> +        pg_stat_get_checkpointer_stat_reset_time() AS stats_reset;
> +

I don't think the backend written stats belong more accurately in
pg_stat_checkpointer than pg_stat_bgwriter.


I continue to be worried about breaking just about any postgres monitoring
setup.

Greetings,

Andres Freund

On Thu, Feb 9, 2023 at 12:33 PM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,

Thanks for looking at this.

> On 2023-02-09 12:21:51 +0530, Bharath Rupireddy wrote:
> > @@ -1105,18 +1105,22 @@ CREATE VIEW pg_stat_archiver AS
> >
> >  CREATE VIEW pg_stat_bgwriter AS
> >      SELECT
> > -        pg_stat_get_bgwriter_timed_checkpoints() AS checkpoints_timed,
> > -        pg_stat_get_bgwriter_requested_checkpoints() AS checkpoints_req,
> > -        pg_stat_get_checkpoint_write_time() AS checkpoint_write_time,
> > -        pg_stat_get_checkpoint_sync_time() AS checkpoint_sync_time,
> > -        pg_stat_get_bgwriter_buf_written_checkpoints() AS buffers_checkpoint,
> >          pg_stat_get_bgwriter_buf_written_clean() AS buffers_clean,
> >          pg_stat_get_bgwriter_maxwritten_clean() AS maxwritten_clean,
> > -        pg_stat_get_buf_written_backend() AS buffers_backend,
> > -        pg_stat_get_buf_fsync_backend() AS buffers_backend_fsync,
> >          pg_stat_get_buf_alloc() AS buffers_alloc,
> >          pg_stat_get_bgwriter_stat_reset_time() AS stats_reset;
> >
> > +CREATE VIEW pg_stat_checkpointer AS
> > +    SELECT
> > +        pg_stat_get_timed_checkpoints() AS timed_checkpoints,
> > +        pg_stat_get_requested_checkpoints() AS requested_checkpoints,
> > +        pg_stat_get_checkpoint_write_time() AS checkpoint_write_time,
> > +        pg_stat_get_checkpoint_sync_time() AS checkpoint_sync_time,
> > +        pg_stat_get_buf_written_checkpoints() AS buffers_written_checkpoints,
> > +        pg_stat_get_buf_written_backend() AS buffers_written_backend,
> > +        pg_stat_get_buf_fsync_backend() AS buffers_fsync_backend,
> > +        pg_stat_get_checkpointer_stat_reset_time() AS stats_reset;
> > +
>
> I don't think the backend written stats belong more accurately in
> pg_stat_checkpointer than pg_stat_bgwriter.

We accumulate buffers_written_backend and buffers_fsync_backend of all
backends under checkpointer stats to show the aggregated results to
the users. I think this is correct because the checkpointer is the one
that processes fsync requests (of course, backends themselves can
fsync when needed, that's what the buffers_fsync_backend shows),
whereas bgwriter doesn't perform IIUC.

> I continue to be worried about breaking just about any postgres monitoring
> setup.

Hm. Yes, it requires minimal and straightforward changes in monitoring
scripts. Please note that we separated out bgwriter and checkpointer
in v9.2 12 years ago but we haven't had a chance to separate the stats
so far. We might do it at some point of time, IMHO this is that time.

We did away with promote_trigger_file (cd4329d) very recently. The
agreement was that the changes required to move on to other mechanisms
of promotion are minimal, hence we didn't want it to be first
deprecated and then removed.

From the discussion upthread, it looks like Robert, Amit, Bertrand,
Greg and myself are in favour of not having a deprecated version but
moving them to the new pg_stat_checkpointer view.

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

Hi,

On 2023-02-09 19:00:00 +0530, Bharath Rupireddy wrote:
> On Thu, Feb 9, 2023 at 12:33 PM Andres Freund <andres@anarazel.de> wrote:
> > On 2023-02-09 12:21:51 +0530, Bharath Rupireddy wrote:
> > > @@ -1105,18 +1105,22 @@ CREATE VIEW pg_stat_archiver AS
> > >
> > >  CREATE VIEW pg_stat_bgwriter AS
> > >      SELECT
> > > -        pg_stat_get_bgwriter_timed_checkpoints() AS checkpoints_timed,
> > > -        pg_stat_get_bgwriter_requested_checkpoints() AS checkpoints_req,
> > > -        pg_stat_get_checkpoint_write_time() AS checkpoint_write_time,
> > > -        pg_stat_get_checkpoint_sync_time() AS checkpoint_sync_time,
> > > -        pg_stat_get_bgwriter_buf_written_checkpoints() AS buffers_checkpoint,
> > >          pg_stat_get_bgwriter_buf_written_clean() AS buffers_clean,
> > >          pg_stat_get_bgwriter_maxwritten_clean() AS maxwritten_clean,
> > > -        pg_stat_get_buf_written_backend() AS buffers_backend,
> > > -        pg_stat_get_buf_fsync_backend() AS buffers_backend_fsync,
> > >          pg_stat_get_buf_alloc() AS buffers_alloc,
> > >          pg_stat_get_bgwriter_stat_reset_time() AS stats_reset;
> > >
> > > +CREATE VIEW pg_stat_checkpointer AS
> > > +    SELECT
> > > +        pg_stat_get_timed_checkpoints() AS timed_checkpoints,
> > > +        pg_stat_get_requested_checkpoints() AS requested_checkpoints,
> > > +        pg_stat_get_checkpoint_write_time() AS checkpoint_write_time,
> > > +        pg_stat_get_checkpoint_sync_time() AS checkpoint_sync_time,
> > > +        pg_stat_get_buf_written_checkpoints() AS buffers_written_checkpoints,
> > > +        pg_stat_get_buf_written_backend() AS buffers_written_backend,
> > > +        pg_stat_get_buf_fsync_backend() AS buffers_fsync_backend,
> > > +        pg_stat_get_checkpointer_stat_reset_time() AS stats_reset;
> > > +
> >
> > I don't think the backend written stats belong more accurately in
> > pg_stat_checkpointer than pg_stat_bgwriter.
> 
> We accumulate buffers_written_backend and buffers_fsync_backend of all
> backends under checkpointer stats to show the aggregated results to
> the users. I think this is correct because the checkpointer is the one
> that processes fsync requests (of course, backends themselves can
> fsync when needed, that's what the buffers_fsync_backend shows),
> whereas bgwriter doesn't perform IIUC.

That's true for buffers_fsync_backend, but not true for
buffers_backend/buffers_written_backend.

That isn't tied to checkpointer.


I think if we end up breaking compat, we should just drop that column. The
pg_stat_io patch from Melanie, which I hope to finish committing by tomorrow,
provides that in a more useful way, in a less confusing place.

I'm not sure it's worth having buffers_fsync_backend in pg_stat_checkpointer
in that case. You can get nearly the same information from pg_stat_io as well
(except fsyncs for SLRUs that couldn't be put into the queue, which you'd not
see right now - hard to believe that ever happens at a relelvant frequency).


> > I continue to be worried about breaking just about any postgres monitoring
> > setup.
> 
> Hm. Yes, it requires minimal and straightforward changes in monitoring
> scripts. Please note that we separated out bgwriter and checkpointer
> in v9.2 12 years ago but we haven't had a chance to separate the stats
> so far. We might do it at some point of time, IMHO this is that time.

> We did away with promote_trigger_file (cd4329d) very recently. The
> agreement was that the changes required to move on to other mechanisms
> of promotion are minimal, hence we didn't want it to be first
> deprecated and then removed.

That's not really comparable, because we have had pg_ctl promote for a long
time. You can use it across all supported versions. pg_promote() is nearly
there as well.  Whereas there's no way to use same query across all versions.

IME there also exist a lot more hand-rolled monitoring setups
than hand-rolled automatic promotion setups.


> From the discussion upthread, it looks like Robert, Amit, Bertrand,
> Greg and myself are in favour of not having a deprecated version but
> moving them to the new pg_stat_checkpointer view.

Yep, and I think you are all wrong, and that this is just going to cause
unnecessary pain :). I'm not going to try to prevent the patch from going in
because of this, just to be clear.

Greetings,

Andres Freund

On Thu, Feb 09, 2023 at 04:46:04PM -0800, Andres Freund wrote:
> I think if we end up breaking compat, we should just drop that
> column.

Indeed.

> Yep, and I think you are all wrong, and that this is just going to cause
> unnecessary pain :). I'm not going to try to prevent the patch from going in
> because of this, just to be clear.

Catalog attributes have faced a lot of renames across the years, with
the same potential of breakages for monitoring tools.  I am not saying
that all of them are justified, but we have usually done so because it
makes sense to reshape things in the way they are now, thinking
long-term.  Splitting pg_stat_bgwriter into two views does not strike
me as something that bad, TBH, because it becomes clearer which stats
are attached to which process (bgwriter or checkpointer).  (Note: I
have not checked in details the stats switching to the new view and
how pertinent each choice is.)
--
Michael

On Fri, Feb 10, 2023 at 6:16 AM Andres Freund <andres@anarazel.de> wrote:
>
> That's true for buffers_fsync_backend, but not true for
> buffers_backend/buffers_written_backend.
>
> That isn't tied to checkpointer.
>
> I think if we end up breaking compat, we should just drop that column. The
> pg_stat_io patch from Melanie, which I hope to finish committing by tomorrow,
> provides that in a more useful way, in a less confusing place.

On Fri, Feb 10, 2023 at 11:29 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Feb 09, 2023 at 04:46:04PM -0800, Andres Freund wrote:
> > I think if we end up breaking compat, we should just drop that
> > column.
>
> Indeed.

Yeah, pg_stat_io is a better place to track the backend IO stats.  I
removed buffers_backend, please see the attached 0001 patch.

On Fri, Feb 10, 2023 at 6:16 AM Andres Freund <andres@anarazel.de> wrote:
>
> I'm not sure it's worth having buffers_fsync_backend in pg_stat_checkpointer
> in that case. You can get nearly the same information from pg_stat_io as well
> (except fsyncs for SLRUs that couldn't be put into the queue, which you'd not
> see right now - hard to believe that ever happens at a relelvant frequency).

I think it'd be better to move the SLRU fsync stats during checkpoints
to the pg_stat_slru view, then it can be a one-stop view to track all
SLRU IO stats. This lets us remove buffers_fsync_backend too, please
see the attached 0002 patch. However, one metric we might miss is the
number of times checkpointer missed to absorb the fsync requests. If
needed, this metric can be added to the new pg_stat_checkpointer view.

> > Yep, and I think you are all wrong, and that this is just going to cause
> > unnecessary pain :). I'm not going to try to prevent the patch from going in
> > because of this, just to be clear.
>
> Catalog attributes have faced a lot of renames across the years, with
> the same potential of breakages for monitoring tools.  I am not saying
> that all of them are justified, but we have usually done so because it
> makes sense to reshape things in the way they are now, thinking
> long-term.  Splitting pg_stat_bgwriter into two views does not strike
> me as something that bad, TBH, because it becomes clearer which stats
> are attached to which process (bgwriter or checkpointer).  (Note: I
> have not checked in details the stats switching to the new view and
> how pertinent each choice is.)

Thanks. FWIW, I've attached the patch introducing pg_stat_checkpointer
as 0003 here.

Please review the attached v7 patch set.

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

On Fri, Feb 10, 2023 at 10:00 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> Thanks. FWIW, I've attached the patch introducing pg_stat_checkpointer
> as 0003 here.
>
> Please review the attached v7 patch set.

Needed a rebase. Please review the attached v8 patch set.

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

On Mon, Feb 13, 2023 at 11:31:03AM +0530, Bharath Rupireddy wrote:
> Needed a rebase. Please review the attached v8 patch set.

I was looking at this patch, and got a few comments.

FWIW, I kind of agree with the feeling of Bertrand upthread that using
"checkpoint_" in the attribute names for the new view is globally
inconsistent.  After 0003, we get:
=# select attname from pg_attribute
     where attrelid = 'pg_stat_checkpointer'::regclass
     and attnum > 0;
          attname
-----------------------------
 timed_checkpoints
 requested_checkpoints
 checkpoint_write_time
 checkpoint_sync_time
 buffers_written_checkpoints
 stats_reset
(6 rows)
=# select attname from pg_attribute
     where attrelid = 'pg_stat_bgwriter'::regclass and
     attnum > 0;
     attname
------------------
 buffers_clean
 maxwritten_clean
 buffers_alloc
 stats_reset
(4 rows)

The view for the bgwriter does not do that.  I'd suggest to use
functions that are named as pg_stat_get_checkpoint_$att with shorter
$atts.  It is true that "timed" is a bit confusing, because it refers
to a number of checkpoints, and that can be read as a time value (?).
So how about num_timed?  And for the others num_requested and
buffers_written?

+ * Unlike the checkpoint fields, reqquests related fields are protected by

s/reqquests/requests/.

 SlruSyncFileTag(SlruCtl ctl, const FileTag *ftag, char *path)
 {
+    SlruShared    shared = ctl->shared;
     int            fd;
     int            save_errno;
     int            result;

+    /* update the stats counter of flushes */
+    pgstat_count_slru_flush(shared->slru_stats_idx);

Why is that in 0002?  Isn't that something we should treat as a bug
fix of its own, even backpatching it to make sure that the flush
requests for individual commit_ts, multixact and clog files are
counted in the stats?

Saying that, I am OK with moving ahead with 0001 and 0002 to remove
buffers_backend_fsync and buffers_backend from pg_stat_bgwriter, but
it is better to merge them into a single commit.  It helps with 0003
and this would encourage the use of pg_stat_io that does a better job
overall with more field granularity.
--
Michael

On Thu, Oct 26, 2023 at 7:30 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> I was looking at this patch, and got a few comments.

Thanks.

> The view for the bgwriter does not do that.  I'd suggest to use
> functions that are named as pg_stat_get_checkpoint_$att with shorter
> $atts.  It is true that "timed" is a bit confusing, because it refers
> to a number of checkpoints, and that can be read as a time value (?).
> So how about num_timed?  And for the others num_requested and
> buffers_written?

+1. PSA v9-0003.

> + * Unlike the checkpoint fields, reqquests related fields are protected by
>
> s/reqquests/requests/.

Fixed.

>  SlruSyncFileTag(SlruCtl ctl, const FileTag *ftag, char *path)
>  {
> +       SlruShared      shared = ctl->shared;
>         int                     fd;
>         int                     save_errno;
>         int                     result;
>
> +       /* update the stats counter of flushes */
> +       pgstat_count_slru_flush(shared->slru_stats_idx);
>
> Why is that in 0002?  Isn't that something we should treat as a bug
> fix of its own, even backpatching it to make sure that the flush
> requests for individual commit_ts, multixact and clog files are
> counted in the stats?

+1. I included the fix in a separate patch 0002 here.

> Saying that, I am OK with moving ahead with 0001 and 0002 to remove
> buffers_backend_fsync and buffers_backend from pg_stat_bgwriter, but
> it is better to merge them into a single commit.  It helps with 0003
> and this would encourage the use of pg_stat_io that does a better job
> overall with more field granularity.

I merged v8 0001 and 0002 into one single patch, PSA v9-0001.

PSA v9 patch set.

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

On Thu, Oct 26, 2023 at 10:55:00PM +0530, Bharath Rupireddy wrote:
> On Thu, Oct 26, 2023 at 7:30 AM Michael Paquier <michael@paquier.xyz> wrote:
>> Why is that in 0002?  Isn't that something we should treat as a bug
>> fix of its own, even backpatching it to make sure that the flush
>> requests for individual commit_ts, multixact and clog files are
>> counted in the stats?
>
> +1. I included the fix in a separate patch 0002 here.

Hmm.  As per the existing call of pgstat_count_slru_flush() in
SimpleLruWriteAll(), routine called SimpleLruFlush() until ~13 and
dee663f78439, an incrementation of 1 for slru_stats_idx refers to all
the flushes for all the dirty data of this SLRU in a single pass.
This addition actually means that we would now increment the counter
for each sync request, changing its meaning.  Perhaps there is an
argument for changing the meaning of this parameter to be based on the
number of flush requests completed, but if that were to happen it
would be better to remove pgstat_count_slru_flush() in
SimpleLruWriteAll(), I guess, or just aggregate this counter once,
which would be cheaper.

>> Saying that, I am OK with moving ahead with 0001 and 0002 to remove
>> buffers_backend_fsync and buffers_backend from pg_stat_bgwriter, but
>> it is better to merge them into a single commit.  It helps with 0003
>> and this would encourage the use of pg_stat_io that does a better job
>> overall with more field granularity.
>
> I merged v8 0001 and 0002 into one single patch, PSA v9-0001.

v9-0001 is OK, so I have applied it.

v9-0003 is mostly a mechanical change, and it passes the eye test.  I
have spotted two nits.

+CREATE VIEW pg_stat_checkpointer AS
+    SELECT
+        pg_stat_get_checkpointer_num_timed() AS num_timed,
+        pg_stat_get_checkpointer_num_requested() AS num_requested,
+        pg_stat_get_checkpointer_write_time() AS write_time,
+        pg_stat_get_checkpointer_sync_time() AS sync_time,
+        pg_stat_get_checkpointer_buffers_written() AS buffers_written,
+        pg_stat_get_checkpointer_stat_reset_time() AS stats_reset;

Okay with this layer.  I am wondering if others have opinions to
share about these names for the attributes of pg_stat_checkpointer,
though.

-   single row, containing global data for the cluster.
+   single row, containing data about the bgwriter of the cluster.

"bgwriter" is used in one place of the docs, so perhaps "background
writer" is better here?

The error message generated for an incorrect target needs to be
updated in pg_stat_reset_shared():
=# select pg_stat_reset_shared('checkpointe');
ERROR:  22023: unrecognized reset target: "checkpointe"
HINT:  Target must be "archiver", "bgwriter", "io", "recovery_prefetch", or "wal".
--
Michael

On Fri, Oct 27, 2023 at 8:03 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> Hmm.  As per the existing call of pgstat_count_slru_flush() in
> SimpleLruWriteAll(), routine called SimpleLruFlush() until ~13 and
> dee663f78439, an incrementation of 1 for slru_stats_idx refers to all
> the flushes for all the dirty data of this SLRU in a single pass.
>
> This addition actually means that we would now increment the counter
> for each sync request, changing its meaning.  Perhaps there is an
> argument for changing the meaning of this parameter to be based on the
> number of flush requests completed, but if that were to happen it
> would be better to remove pgstat_count_slru_flush() in
> SimpleLruWriteAll(), I guess, or just aggregate this counter once,
> which would be cheaper.

Right. Interestingly, there are 2 SLRU flush related wait events
WAIT_EVENT_SLRU_SYNC ("Waiting for SLRU data to reach durable storage
following a page write") and WAIT_EVENT_SLRU_FLUSH_SYNC ("Waiting for
SLRU data to reach durable storage during a checkpoint or database
shutdown"). And, we're counting the SLRU flushes in two of these
places into one single stat variable. These two events look confusing
and may be useful if shown in an aggregated way.

A possible way is to move existing pgstat_count_slru_flush in
SimpleLruWriteAll closer to pg_fsync and WAIT_EVENT_SLRU_SYNC in
SlruPhysicalWritePage, remove WAIT_EVENT_SLRU_FLUSH_SYNC completely,
use WAIT_EVENT_SLRU_SYNC in SlruSyncFileTag and count the flushes in
SlruSyncFileTag. This aggregated way is much simpler IMV.

Another possible way is to have separate stat variables for each of
the SLRU flushes WAIT_EVENT_SLRU_SYNC and WAIT_EVENT_SLRU_FLUSH_SYNC
and expose them separately in pg_stat_slru. I don't like this
approach.

> v9-0003 is mostly a mechanical change, and it passes the eye test.

Thanks. Indeed it contains mechanical changes.

> I have spotted two nits.
>
> +CREATE VIEW pg_stat_checkpointer AS
> +    SELECT
> +        pg_stat_get_checkpointer_num_timed() AS num_timed,
> +        pg_stat_get_checkpointer_num_requested() AS num_requested,
> +        pg_stat_get_checkpointer_write_time() AS write_time,
> +        pg_stat_get_checkpointer_sync_time() AS sync_time,
> +        pg_stat_get_checkpointer_buffers_written() AS buffers_written,
> +        pg_stat_get_checkpointer_stat_reset_time() AS stats_reset;
>
> Okay with this layer.  I am wondering if others have opinions to
> share about these names for the attributes of pg_stat_checkpointer,
> though.

I think these column names are a good fit in this context as we can't
just call timed/requested/write/sync and having "checkpoint" makes the
column names long unnecessarily. FWIW, I see some of the user-exposed
field names starting with num_* (num_nonnulls, num_nulls, num_lwlocks,
num_transactions).

> -   single row, containing global data for the cluster.
> +   single row, containing data about the bgwriter of the cluster.
>
> "bgwriter" is used in one place of the docs, so perhaps "background
> writer" is better here?

+1. Changed in the attached v10-0001.

> The error message generated for an incorrect target needs to be
> updated in pg_stat_reset_shared():
> =# select pg_stat_reset_shared('checkpointe');
> ERROR:  22023: unrecognized reset target: "checkpointe"
> HINT:  Target must be "archiver", "bgwriter", "io", "recovery_prefetch", or "wal".

+1. Changed in the attached v10-001. FWIW, having a test case in
stats.sql emitting this error message and hint would have helped here.
If okay, I can add one.

PS: I'll park the SLRU flush related patch aside until the approach is
finalized. I'm posting the pg_stat_checkpointer patch as v10-0001.

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

On Fri, Oct 27, 2023 at 10:23:34AM +0530, Bharath Rupireddy wrote:
> A possible way is to move existing pgstat_count_slru_flush in
> SimpleLruWriteAll closer to pg_fsync and WAIT_EVENT_SLRU_SYNC in
> SlruPhysicalWritePage, remove WAIT_EVENT_SLRU_FLUSH_SYNC completely,
> use WAIT_EVENT_SLRU_SYNC in SlruSyncFileTag and count the flushes in
> SlruSyncFileTag. This aggregated way is much simpler IMV.
>
> Another possible way is to have separate stat variables for each of
> the SLRU flushes WAIT_EVENT_SLRU_SYNC and WAIT_EVENT_SLRU_FLUSH_SYNC
> and expose them separately in pg_stat_slru. I don't like this
> approach.

This touches an area covered by a different patch, registered in this
commit fest as well:
https://www.postgresql.org/message-id/CAMm1aWb18EpT0whJrjG+-nyhNouXET6ZUw0pNYYAe+NezpvsAA@mail.gmail.com

So perhaps we'd better move the discussion there.  The patch posted
there is going to need a rebase anyway once the split with
pg_stat_checkpointer is introduced.

>> The error message generated for an incorrect target needs to be
>> updated in pg_stat_reset_shared():
>> =# select pg_stat_reset_shared('checkpointe');
>> ERROR:  22023: unrecognized reset target: "checkpointe"
>> HINT:  Target must be "archiver", "bgwriter", "io", "recovery_prefetch", or "wal".
>
> +1. Changed in the attached v10-001. FWIW, having a test case in
> stats.sql emitting this error message and hint would have helped here.
> If okay, I can add one.
>
> PS: I'll park the SLRU flush related patch aside until the approach is
> finalized. I'm posting the pg_stat_checkpointer patch as v10-0001.

Thanks.  That seems OK.  I don't have the wits to risk my weekend on
buildfarm failures if any, so that will have to wait a bit.
--
Michael

On Fri, Oct 27, 2023 at 10:32 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Oct 27, 2023 at 10:23:34AM +0530, Bharath Rupireddy wrote:
> > A possible way is to move existing pgstat_count_slru_flush in
> > SimpleLruWriteAll closer to pg_fsync and WAIT_EVENT_SLRU_SYNC in
> > SlruPhysicalWritePage, remove WAIT_EVENT_SLRU_FLUSH_SYNC completely,
> > use WAIT_EVENT_SLRU_SYNC in SlruSyncFileTag and count the flushes in
> > SlruSyncFileTag. This aggregated way is much simpler IMV.
> >
> > Another possible way is to have separate stat variables for each of
> > the SLRU flushes WAIT_EVENT_SLRU_SYNC and WAIT_EVENT_SLRU_FLUSH_SYNC
> > and expose them separately in pg_stat_slru. I don't like this
> > approach.
>
> This touches an area covered by a different patch, registered in this
> commit fest as well:
> https://www.postgresql.org/message-id/CAMm1aWb18EpT0whJrjG+-nyhNouXET6ZUw0pNYYAe+NezpvsAA@mail.gmail.com
>
> So perhaps we'd better move the discussion there.  The patch posted
> there is going to need a rebase anyway once the split with
> pg_stat_checkpointer is introduced.

Yeah, I'll re-look at the SLRU stuff and the other thread next week.

> > finalized. I'm posting the pg_stat_checkpointer patch as v10-0001.
>
> Thanks.  That seems OK.  I don't have the wits to risk my weekend on
> buildfarm failures if any, so that will have to wait a bit.

Hm, okay :)

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

On Fri, Oct 27, 2023 at 10:23:34AM +0530, Bharath Rupireddy wrote:
> +1. Changed in the attached v10-001. FWIW, having a test case in
> stats.sql emitting this error message and hint would have helped here.
> If okay, I can add one.

That may be something to do.  At least it was missed on this thread,
even if we don't add a new  pgstat shared type every day.

> PS: I'll park the SLRU flush related patch aside until the approach is
> finalized. I'm posting the pg_stat_checkpointer patch as v10-0001.

+-- Test that reset_shared with checkpointer specified as the stats type works
+SELECT stats_reset AS checkpointer_reset_ts FROM pg_stat_checkpointer \gset
+SELECT pg_stat_reset_shared('checkpointer');
+SELECT stats_reset > :'checkpointer_reset_ts'::timestamptz FROM pg_stat_checkpointer;
+SELECT stats_reset AS checkpointer_reset_ts FROM pg_stat_checkpointer \gset

Note that you have forgotten to update the test of
pg_stat_reset_shared(NULL) to check for the value of
checkpointer_reset_ts.  I've added an extra SELECT to check that for
pg_stat_checkpointer, and applied v8.
--
Michael

On Mon, Oct 30, 2023 at 6:19 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Oct 27, 2023 at 10:23:34AM +0530, Bharath Rupireddy wrote:
> > +1. Changed in the attached v10-001. FWIW, having a test case in
> > stats.sql emitting this error message and hint would have helped here.
> > If okay, I can add one.
>
> That may be something to do.  At least it was missed on this thread,
> even if we don't add a new  pgstat shared type every day.

Right. Adding test coverage for the error-case is no bad IMV
(https://coverage.postgresql.org/src/backend/utils/adt/pgstatfuncs.c.gcov.html).
Here's the attached 0001 patch for that.

> > PS: I'll park the SLRU flush related patch aside until the approach is
> > finalized. I'm posting the pg_stat_checkpointer patch as v10-0001.
>
> +-- Test that reset_shared with checkpointer specified as the stats type works
> +SELECT stats_reset AS checkpointer_reset_ts FROM pg_stat_checkpointer \gset
> +SELECT pg_stat_reset_shared('checkpointer');
> +SELECT stats_reset > :'checkpointer_reset_ts'::timestamptz FROM pg_stat_checkpointer;
> +SELECT stats_reset AS checkpointer_reset_ts FROM pg_stat_checkpointer \gset
>
> Note that you have forgotten to update the test of
> pg_stat_reset_shared(NULL) to check for the value of
> checkpointer_reset_ts.  I've added an extra SELECT to check that for
> pg_stat_checkpointer, and applied v8.

Oh, thanks for taking care of this. While at it, I noticed that
there's no coverage for pg_stat_reset_shared('recovery_prefetch') and
XLogPrefetchResetStats()
https://coverage.postgresql.org/src/backend/access/transam/xlogprefetcher.c.gcov.html.
Most of the recovery_prefetch code is covered with recovery/streaming
related tests, but the reset stats part is missing. So, I've added
coverage for it in the attached 0002.

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

On Mon, Oct 30, 2023 at 11:59:10AM +0530, Bharath Rupireddy wrote:
> Oh, thanks for taking care of this. While at it, I noticed that
> there's no coverage for pg_stat_reset_shared('recovery_prefetch') and
> XLogPrefetchResetStats()
> https://coverage.postgresql.org/src/backend/access/transam/xlogprefetcher.c.gcov.html.
> Most of the recovery_prefetch code is covered with recovery/streaming
> related tests, but the reset stats part is missing. So, I've added
> coverage for it in the attached 0002.

Indeed, good catch.  I didn't notice the hole in the coverage reports.
I have merged 0001 and 0002 together, and applied them as of
5b2147d9fcc1.
--
Michael