Introduce a new view for checkpointer related stats

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 obvious 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" for pg_stat_bgwriter too?

+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 to a 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 seems obvious 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" for pg_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 to a 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
/messages/by-id/20221121003815.qnwlnz2lhkow2e5w@awork3.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
/messages/by-id/CALj2ACX8jFET1C3bs_edz_8JRcMg5nz8Y7ryjGaCsfnVpAYoVQ@mail.gmail.com
and here's the v3 patch that deprecates
/messages/by-id/CALj2ACUjEvPQYGJHmH2FrAj1gmvHskNrOeNUr7xnwjtkYVZvEQ@mail.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 part 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;
+

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 were part 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 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?

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