Re: Introduce a new view for checkpointer related stats
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
signature.asc
Description: PGP signature
Re: Introduce a new view for checkpointer related stats
On Mon, Oct 30, 2023 at 6:19 AM Michael Paquier 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
v1-0001-Add-error-case-test-for-pg_stat_reset_shared-with.patch
Description: Binary data
v1-0002-Add-test-for-resetting-recovery_prefetch-shared-s.patch
Description: Binary data
Re: Introduce a new view for checkpointer related stats
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
signature.asc
Description: PGP signature
Re: Introduce a new view for checkpointer related stats
On Fri, Oct 27, 2023 at 10:32 AM Michael Paquier 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+nezpv...@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
Re: Introduce a new view for checkpointer related stats
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+nezpv...@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
signature.asc
Description: PGP signature
Re: Introduce a new view for checkpointer related stats
On Fri, Oct 27, 2023 at 8:03 AM Michael Paquier 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
From d64200c575bf8c9f54bdefad95f84dff5a8381eb Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy
Date: Fri, 27 Oct 2023 04:42:55 +
Subject: [PATCH v10] Introduce a new view for checkpointer related stats
pg_stat_bgwriter view currently reports checkpointer stats as
well. It has been that way so far because historically
checkpointer was part of bgwriter until the commits 806a2ae and
bf405ba separated them out way back in PG 9.2.
It is time for us to separate checkpointer stats to its own view
called pg_stat_checkpointer.
Bump catalog version.
Author: Bharath Rupireddy
Reviewed-by: Bertrand Drouvot, Andres Freund
Discussion: https://www.postgresql.org/message-id/CALj2ACVxX2ii%3D66RypXRweZe2EsBRiPMj0aHfRfHUeXJcC7kHg%40mail.gmail.com
---
doc/src/sgml/monitoring.sgml | 98 ++-
src/backend/access/transam/xlog.c | 4 +-
src/backend/catalog/system_views.sql | 14 ++-
src/backend/postmaster/checkpoi
Re: Introduce a new view for checkpointer related stats
On Thu, Oct 26, 2023 at 10:55:00PM +0530, Bharath Rupireddy wrote:
> On Thu, Oct 26, 2023 at 7:30 AM Michael Paquier 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
signature.asc
Description: PGP signature
Re: Introduce a new view for checkpointer related stats
On Thu, Oct 26, 2023 at 7:30 AM Michael Paquier 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
v9-0001-Do-not-count-backend-writes-and-fsycns-in-checkpo.patch
Description: Binary data
v9-0002-Count-SLRU-page-flushes-during-checkpoint-or-data.patch
Description: Binary data
v9-0003-Introduce-a-new-view-for-checkpointer-related-sta.patch
Description: Binary data
Re: Introduce a new view for checkpointer related stats
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
signature.asc
Description: PGP signature
Re: Introduce a new view for checkpointer related stats
c.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5704,11 +5704,6 @@
proname => 'pg_stat_get_checkpoint_sync_time', provolatile => 's',
proparallel => 'r', prorettype => 'float8', proargtypes => '',
prosrc => 'pg_stat_get_checkpoint_sync_time' },
-{ oid => '3063',
- descr => 'statistics: number of backend buffer writes that did their own fsync',
- proname => 'pg_stat_get_buf_fsync_backend', provolatile => 's',
- proparallel => 'r', prorettype => 'int8', proargtypes => '',
- prosrc => 'pg_stat_get_buf_fsync_backend' },
{ oid => '2859', descr => 'statistics: number of buffer allocations',
proname => 'pg_stat_get_buf_alloc', provolatile => 's', proparallel => 'r',
prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_buf_alloc' },
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index d1675909db..28fb6292d9 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -273,7 +273,6 @@ typedef struct PgStat_CheckpointerStats
PgStat_Counter checkpoint_write_time; /* times in milliseconds */
PgStat_Counter checkpoint_sync_time;
PgStat_Counter buf_written_checkpoints;
- PgStat_Counter buf_fsync_backend;
} PgStat_CheckpointerStats;
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 5b4f441245..9c9adb6140 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1821,7 +1821,6 @@ pg_stat_bgwriter| SELECT pg_stat_get_bgwriter_timed_checkpoints() AS checkpoints
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_fsync_backend() AS buffers_backend_fsync,
pg_stat_get_buf_alloc() AS buffers_alloc,
pg_stat_get_bgwriter_stat_reset_time() AS stats_reset;
pg_stat_database| SELECT oid AS datid,
--
2.34.1
From 5b349d6b3a3b8db1498e038aed81909fd1f15fe4 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy
Date: Mon, 13 Feb 2023 05:05:07 +
Subject: [PATCH v8] Introduce a new view for checkpointer related stats
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.
It is time for us to separate checkpointer stats to its own view
called pg_stat_checkpointer.
Bump catalog version.
---
doc/src/sgml/monitoring.sgml | 98 ++-
src/backend/catalog/system_views.sql | 14 ++-
.../utils/activity/pgstat_checkpointer.c | 1 +
src/backend/utils/adt/pgstatfuncs.c | 19 ++--
src/include/catalog/pg_proc.dat | 22 +++--
src/include/pgstat.h | 1 +
src/test/recovery/t/029_stats_restart.pl | 6 +-
src/test/regress/expected/rules.out | 13 +--
src/test/regress/expected/stats.out | 21 +++-
src/test/regress/sql/stats.sql| 12 ++-
10 files changed, 146 insertions(+), 61 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 098ca5999d..47bf6ff247 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -451,6 +451,15 @@ postgres 27093 0.0 0.0 30096 2752 ?Ss 11:34 0:00 postgres: ser
+
+ pg_stat_checkpointerpg_stat_checkpointer
+ One row only, showing statistics about the
+ checkpointer process's activity. See
+
+ pg_stat_checkpointer for details.
+
+
+
pg_stat_databasepg_stat_database
One row per database, showing database-wide statistics. See
@@ -3974,7 +3983,7 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
The pg_stat_bgwriter view will always have a
- single row, containing global data for the cluster.
+ single row, containing data about the bgwriter of the cluster.
@@ -3994,77 +4003,118 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
- checkpoints_timed bigint
+ buffers_clean bigint
- Number of scheduled checkpoints that have been performed
+ Number of buffers written by the background writer
- checkpoints_req bigint
+ maxwritten_clean bigint
- Number of requested checkpoints that have been performed
+ Number of times the background writer stopped a cleaning
+ scan because it had written too many buffers
- checkpoint_write_time double precision
+ buffers_alloc bigint
- Total amount of time that has been spent in the portion of
- checkpoint processing where files are written to disk, in milliseconds
+ Number of
Re: Introduce a new view for checkpointer related stats
ckpointerShmem->num_requests >= CheckpointerShmem->max_requests &&
!CompactCheckpointerRequestQueue()))
{
- /*
- * Count the subset of writes where backends have to do their own
- * fsync
- */
- if (!AmBackgroundWriterProcess())
- CheckpointerShmem->num_backend_fsync++;
LWLockRelease(CheckpointerCommLock);
return false;
}
@@ -1264,12 +1252,6 @@ AbsorbSyncRequests(void)
LWLockAcquire(CheckpointerCommLock, LW_EXCLUSIVE);
- /* Transfer stats counts into pending pgstats message */
- PendingCheckpointerStats.buf_fsync_backend
- += CheckpointerShmem->num_backend_fsync;
-
- CheckpointerShmem->num_backend_fsync = 0;
-
/*
* We try to avoid holding the lock for a long time by copying the request
* array, and processing the requests after releasing the lock.
diff --git a/src/backend/utils/activity/pgstat_checkpointer.c b/src/backend/utils/activity/pgstat_checkpointer.c
index a048205d6e..03ed5a 100644
--- a/src/backend/utils/activity/pgstat_checkpointer.c
+++ b/src/backend/utils/activity/pgstat_checkpointer.c
@@ -52,7 +52,6 @@ pgstat_report_checkpointer(void)
CHECKPOINTER_ACC(checkpoint_write_time);
CHECKPOINTER_ACC(checkpoint_sync_time);
CHECKPOINTER_ACC(buf_written_checkpoints);
- CHECKPOINTER_ACC(buf_fsync_backend);
#undef CHECKPOINTER_ACC
pgstat_end_changecount_write(_shmem->changecount);
@@ -119,6 +118,5 @@ pgstat_checkpointer_snapshot_cb(void)
CHECKPOINTER_COMP(checkpoint_write_time);
CHECKPOINTER_COMP(checkpoint_sync_time);
CHECKPOINTER_COMP(buf_written_checkpoints);
- CHECKPOINTER_COMP(buf_fsync_backend);
#undef CHECKPOINTER_COMP
}
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index ce16884f1b..7f4c39f052 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1227,12 +1227,6 @@ pg_stat_get_bgwriter_stat_reset_time(PG_FUNCTION_ARGS)
PG_RETURN_TIMESTAMPTZ(pgstat_fetch_stat_bgwriter()->stat_reset_timestamp);
}
-Datum
-pg_stat_get_buf_fsync_backend(PG_FUNCTION_ARGS)
-{
- PG_RETURN_INT64(pgstat_fetch_stat_checkpointer()->buf_fsync_backend);
-}
-
Datum
pg_stat_get_buf_alloc(PG_FUNCTION_ARGS)
{
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 3188d9ee1f..84bacd0329 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5704,11 +5704,6 @@
proname => 'pg_stat_get_checkpoint_sync_time', provolatile => 's',
proparallel => 'r', prorettype => 'float8', proargtypes => '',
prosrc => 'pg_stat_get_checkpoint_sync_time' },
-{ oid => '3063',
- descr => 'statistics: number of backend buffer writes that did their own fsync',
- proname => 'pg_stat_get_buf_fsync_backend', provolatile => 's',
- proparallel => 'r', prorettype => 'int8', proargtypes => '',
- prosrc => 'pg_stat_get_buf_fsync_backend' },
{ oid => '2859', descr => 'statistics: number of buffer allocations',
proname => 'pg_stat_get_buf_alloc', provolatile => 's', proparallel => 'r',
prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_buf_alloc' },
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index d1675909db..28fb6292d9 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -273,7 +273,6 @@ typedef struct PgStat_CheckpointerStats
PgStat_Counter checkpoint_write_time; /* times in milliseconds */
PgStat_Counter checkpoint_sync_time;
PgStat_Counter buf_written_checkpoints;
- PgStat_Counter buf_fsync_backend;
} PgStat_CheckpointerStats;
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index c98f9cd747..d32f47cdb0 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1821,7 +1821,6 @@ pg_stat_bgwriter| SELECT pg_stat_get_bgwriter_timed_checkpoints() AS checkpoints
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_fsync_backend() AS buffers_backend_fsync,
pg_stat_get_buf_alloc() AS buffers_alloc,
pg_stat_get_bgwriter_stat_reset_time() AS stats_reset;
pg_stat_database| SELECT oid AS datid,
--
2.34.1
From c7dd583d288add34a7fdb412675d2b6258480f1e Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy
Date: Fri, 10 Feb 2023 16:04:27 +
Subject: [PATCH v7] Introduce a new view for checkpointer related stats
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.
It is time for us to separate checkpointer stats to its own view
called pg_stat_checkpointer.
Bump catalog version.
---
doc/src/sgml/monitoring.sgml | 98 ++-
src/backend/catalog/system_views.sql | 14
Re: Introduce a new view for checkpointer related stats
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 signature.asc Description: PGP signature
Re: Introduce a new view for checkpointer related stats
Hi, On 2023-02-09 19:00:00 +0530, Bharath Rupireddy wrote: > On Thu, Feb 9, 2023 at 12:33 PM Andres Freund 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
Re: Introduce a new view for checkpointer related stats
On Thu, Feb 9, 2023 at 12:33 PM Andres Freund 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
Re: Introduce a new view for checkpointer related stats
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
Re: Introduce a new view for checkpointer related stats
On Sat, Jan 21, 2023 at 5:56 AM Bharath Rupireddy wrote: > > On Fri, Dec 2, 2022 at 1:07 PM Drouvot, Bertrand > 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 From 87829fec1f51a2ed764c6e48825caf13ef4b2725 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy Date: Thu, 9 Feb 2023 06:25:35 + Subject: [PATCH v6] Introduce a new view for checkpointer related stats 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. It is time for us to separate checkpointer stats to its own view called pg_stat_checkpointer. Bump catalog version. --- doc/src/sgml/monitoring.sgml | 110 +- src/backend/catalog/system_views.sql | 18 +-- .../utils/activity/pgstat_checkpointer.c | 1 + src/backend/utils/adt/pgstatfuncs.c | 19 +-- src/include/catalog/pg_proc.dat | 22 ++-- src/include/pgstat.h | 1 + src/test/recovery/t/029_stats_restart.pl | 6 +- src/test/regress/expected/rules.out | 17 +-- src/test/regress/expected/stats.out | 21 +++- src/test/regress/sql/stats.sql| 12 +- 10 files changed, 156 insertions(+), 71 deletions(-) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index b246ddc634..f53f5769fc 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -494,6 +494,15 @@ postgres 27093 0.0 0.0 30096 2752 ?Ss 11:34 0:00 postgres: ser + + pg_stat_checkpointerpg_stat_checkpointer + One row only, showing statistics about the + checkpointer process's activity. See + + pg_stat_checkpointer for details. + + + pg_stat_walpg_stat_wal One row only, showing statistics about WAL activity. See @@ -3681,7 +3690,7 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i The pg_stat_bgwriter view will always have a - single row, containing global data for the cluster. + single row, containing data about the bgwriter of the cluster. @@ -3701,97 +3710,138 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i - checkpoints_timed bigint + buffers_clean bigint - Number of scheduled checkpoints that have been performed + Number of buffers written by the background writer - checkpoints_req bigint + maxwritten_clean bigint - Number of requested checkpoints that have been performed + Number of times the background writer stopped a cleaning + scan because it had written too many buffers - checkpoint_write_time double precision + buffers_alloc bigint - Total amount of time that has been spent in the portion of - checkpoint processing where files are written to disk, in milliseconds + Number of buffers allocated - checkpoint_sync_time double precision + stats_reset timestamp with time zone - Total amount of time that has been spent in the portion of - checkpoint processing where files are synchronized to disk, in - milliseconds + Time at which these statistics were last reset + + + + + + + + pg_stat_checkpointer + + + pg_stat_checkpointer + + + + The pg_stat_checkpointer view will always have a + single row, containing data about the checkpointer process of the cluster. + + + pg_stat_checkpointer View + + - buffers_checkpoint bigint + Column Type - Number of buffers written during checkpoints + Description + + - buffers_clean bigint + timed_checkpoints bigint - Number of buffers written by the background writer + Number of scheduled checkpoints that have been performed - maxwritten_clean bigint + requested_checkpoints bigint - Number of times the background writer stopped a cleaning - scan because it had written too many buffers + Number of requested checkpoints that have been performed - buffers_backend bigint + checkpoint_write_time double precision -
Re: Introduce a new view for checkpointer related stats
On Fri, Dec 2, 2022 at 1:07 PM Drouvot, Bertrand 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 From e705916c40649d13a66299159cc992d0572960f5 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy Date: Fri, 20 Jan 2023 14:20:03 + Subject: [PATCH v5] Introduce a new view for checkpointer related stats 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. It is time for us to separate checkpointer stats to its own view called pg_stat_checkpointer. Bump catalog version. --- doc/src/sgml/monitoring.sgml | 110 +- src/backend/catalog/system_views.sql | 18 +-- .../utils/activity/pgstat_checkpointer.c | 1 + src/backend/utils/adt/pgstatfuncs.c | 19 +-- src/include/catalog/pg_proc.dat | 22 ++-- src/include/pgstat.h | 1 + src/test/recovery/t/029_stats_restart.pl | 6 +- src/test/regress/expected/rules.out | 17 +-- src/test/regress/expected/stats.out | 21 +++- src/test/regress/sql/stats.sql| 12 +- 10 files changed, 156 insertions(+), 71 deletions(-) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index e3a783abd0..54166d88fc 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -494,6 +494,15 @@ postgres 27093 0.0 0.0 30096 2752 ?Ss 11:34 0:00 postgres: ser + + pg_stat_checkpointerpg_stat_checkpointer + One row only, showing statistics about the + checkpointer process's activity. See + + pg_stat_checkpointer for details. + + + pg_stat_walpg_stat_wal One row only, showing statistics about WAL activity. See @@ -3671,7 +3680,7 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i The pg_stat_bgwriter view will always have a - single row, containing global data for the cluster. + single row, containing data about the bgwriter of the cluster. @@ -3691,97 +3700,138 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i - checkpoints_timed bigint + buffers_clean bigint - Number of scheduled checkpoints that have been performed + Number of buffers written by the background writer - checkpoints_req bigint + maxwritten_clean bigint - Number of requested checkpoints that have been performed + Number of times the background writer stopped a cleaning + scan because it had written too many buffers - checkpoint_write_time double precision + buffers_alloc bigint - Total amount of time that has been spent in the portion of - checkpoint processing where files are written to disk, in milliseconds + Number of buffers allocated - checkpoint_sync_time double precision + stats_reset timestamp with time zone - Total amount of time that has been spent in the portion of - checkpoint processing where files are synchronized to disk, in - milliseconds + Time at which these statistics were last reset + + + + + + + + pg_stat_checkpointer + + + pg_stat_checkpointer + + + + The pg_stat_checkpointer view will always have a + single row, containing data about the checkpointer process of the cluster. + + + pg_stat_checkpointer View + + - buffers_checkpoint bigint + Column Type - Number of buffers written during checkpoints + Description + + - buffers_clean bigint + timed_checkpoints bigint - Number of buffers written by the background writer + Number of scheduled checkpoints that have been performed - maxwritten_clean bigint + requested_checkpoints bigint - Number of times the background writer stopped a cleaning - scan because it had written too many buffers + Number of requested checkpoints that have been performed - buffers_backend bigint + checkpoint_write_time double precision - Number of buffers written directly by a backend + Total amount of time that has been spent in the portion of + checkpoint processing where files are written to disk, in millis
Re: Introduce a new view for checkpointer related stats
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
Re: Introduce a new view for checkpointer related stats
The patch looks good to me. Thanks & Regards, Nitin Jadhav On Fri, Dec 2, 2022 at 11:20 AM Bharath Rupireddy wrote: > > On Wed, Nov 30, 2022 at 5:15 PM Bharath Rupireddy > 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
Re: Introduce a new view for checkpointer related stats
Hi, On 12/2/22 6:50 AM, Bharath Rupireddy wrote: On Wed, Nov 30, 2022 at 5:15 PM Bharath Rupireddy 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
Re: Introduce a new view for checkpointer related stats
On Fri, Dec 2, 2022 at 12:54 PM sirisha chamarthi wrote: > > On Thu, Dec 1, 2022 at 9:50 PM Bharath Rupireddy > wrote: >> >> On Wed, Nov 30, 2022 at 5:15 PM Bharath Rupireddy >> 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
Re: Introduce a new view for checkpointer related stats
On Thu, Dec 1, 2022 at 9:50 PM Bharath Rupireddy < bharath.rupireddyforpostg...@gmail.com> wrote: > On Wed, Nov 30, 2022 at 5:15 PM Bharath Rupireddy > 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? > -- > Bharath Rupireddy > PostgreSQL Contributors Team > RDS Open Source Databases > Amazon Web Services: https://aws.amazon.com >
Re: Introduce a new view for checkpointer related stats
On Wed, Nov 30, 2022 at 5:15 PM Bharath Rupireddy 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 From 8280223b2b8fde888dd8540328336421bbf6138c Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy Date: Fri, 2 Dec 2022 04:58:46 + Subject: [PATCH v4] Introduce a new view for checkpointer related stats 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. It is time for us to separate checkpointer stats to its own view called pg_stat_checkpointer. Bump catalog version. --- doc/src/sgml/monitoring.sgml | 110 +- src/backend/catalog/system_views.sql | 18 +-- .../utils/activity/pgstat_checkpointer.c | 1 + src/backend/utils/adt/pgstatfuncs.c | 19 +-- src/include/catalog/pg_proc.dat | 22 ++-- src/include/pgstat.h | 1 + src/test/recovery/t/029_stats_restart.pl | 6 +- src/test/regress/expected/rules.out | 17 +-- src/test/regress/expected/stats.out | 21 +++- src/test/regress/sql/stats.sql| 12 +- 10 files changed, 156 insertions(+), 71 deletions(-) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 11a8ebe5ec..1279b47d27 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -448,6 +448,15 @@ postgres 27093 0.0 0.0 30096 2752 ?Ss 11:34 0:00 postgres: ser + + pg_stat_checkpointerpg_stat_checkpointer + One row only, showing statistics about the + checkpointer process's activity. See + + pg_stat_checkpointer for details. + + + pg_stat_walpg_stat_wal One row only, showing statistics about WAL activity. See @@ -3633,7 +3642,7 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i The pg_stat_bgwriter view will always have a - single row, containing global data for the cluster. + single row, containing data about the bgwriter of the cluster. @@ -3653,97 +3662,138 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i - checkpoints_timed bigint + buffers_clean bigint - Number of scheduled checkpoints that have been performed + Number of buffers written by the background writer - checkpoints_req bigint + maxwritten_clean bigint - Number of requested checkpoints that have been performed + Number of times the background writer stopped a cleaning + scan because it had written too many buffers - checkpoint_write_time double precision + buffers_alloc bigint - Total amount of time that has been spent in the portion of - checkpoint processing where files are written to disk, in milliseconds + Number of buffers allocated - checkpoint_sync_time double precision + stats_reset timestamp with time zone - Total amount of time that has been spent in the portion of - checkpoint processing where files are synchronized to disk, in - milliseconds + Time at which these statistics were last reset + + + + + + + + pg_stat_checkpointer + + + pg_stat_checkpointer + + + + The pg_stat_checkpointer view will always have a + single row, containing data about the checkpointer process of the cluster. + + + pg_stat
Re: Introduce a new view for checkpointer related stats
On Wed, Nov 30, 2022 at 12:44 PM Drouvot, Bertrand
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
Re: Introduce a new view for checkpointer related stats
Hi, On 11/30/22 7:34 AM, Bharath Rupireddy wrote: On Wed, Nov 30, 2022 at 6:01 AM Andres Freund wrote: Hi, On 2022-11-28 12:58:48 -0500, Robert Haas wrote: On Tue, Nov 22, 2022 at 3:53 PM Andres Freund 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
Re: Introduce a new view for checkpointer related stats
On Wed, Nov 30, 2022 at 6:01 AM Andres Freund wrote: > > Hi, > > On 2022-11-28 12:58:48 -0500, Robert Haas wrote: > > On Tue, Nov 22, 2022 at 3:53 PM Andres Freund 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 v2-0001-Introduce-a-new-view-for-checkpointer-related-sta.patch Description: Binary data
Re: Introduce a new view for checkpointer related stats
Hi, On 2022-11-28 12:58:48 -0500, Robert Haas wrote: > On Tue, Nov 22, 2022 at 3:53 PM Andres Freund 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
Re: Introduce a new view for checkpointer related stats
On Mon, 28 Nov 2022 at 13:00, Robert Haas wrote: > > On Tue, Nov 22, 2022 at 3:53 PM Andres Freund 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
Re: Introduce a new view for checkpointer related stats
Hi, On 11/28/22 6:58 PM, Robert Haas wrote: On Tue, Nov 22, 2022 at 3:53 PM Andres Freund 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
Re: Introduce a new view for checkpointer related stats
On Mon, Nov 28, 2022 at 11:29 PM Robert Haas wrote: > > On Tue, Nov 22, 2022 at 3:53 PM Andres Freund 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.
Re: Introduce a new view for checkpointer related stats
On Mon, Nov 28, 2022 at 11:29 PM Robert Haas wrote: > > On Tue, Nov 22, 2022 at 3:53 PM Andres Freund 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
Re: Introduce a new view for checkpointer related stats
On Tue, Nov 22, 2022 at 3:53 PM Andres Freund 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
Re: Introduce a new view for checkpointer related stats
On Sat, Nov 26, 2022 at 4:32 AM Andres Freund 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 From ec09bfcde836cb23f090fd6088b89f0fb9a68000 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy Date: Mon, 28 Nov 2022 10:07:55 + Subject: [PATCH v3] Introduce a new view for checkpointer related stats 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. It is time for us to separate checkpointer stats to its own view called pg_stat_checkpointer. For now, we deprecate the use of these checkpointer related fields under pg_stat_bgwriter view for smoother transitioning. Eventually, we need to remove them from pg_stat_bgwriter view. Bump catalog version. Author: Bharath Rupireddy Reviewed-by: Andres Freund, Bertrand Drouvot Discussion: https://www.postgresql.org/message-id/CALj2ACVxX2ii=66RypXRweZe2EsBRiPMj0aHfRfHUeXJcC7kHg@mail.gmail.com --- doc/src/sgml/monitoring.sgml | 165 +- src/backend/catalog/system_views.sql | 17 +- .../utils/activity/pgstat_checkpointer.c | 1 + src/backend/utils/adt/pgstatfuncs.c | 19 +- src/include/catalog/pg_proc.dat | 22 ++- src/include/pgstat.h | 1 + src/test/recovery/t/029_stats_restart.pl | 6 +- src/test/regress/expected/rules.out | 14 +- src/test/regress/expected/stats.out | 21 ++- src/test/regress/sql/stats.sql| 12 +- 10 files changed, 236 insertions(+), 42 deletions(-) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 5579b8b9e0..c69d3b21c0 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -448,6 +448,15 @@ postgres 27093 0.0 0.0 30096 2752 ?Ss 11:34 0:00 postgres: ser + + pg_stat_checkpointerpg_stat_checkpointer + One row only, showing statistics about the + checkpointer process's activity. See + + pg_stat_checkpointer for details. + + + pg_stat_walpg_stat_wal One row only, showing statistics about WAL activity. See @@ -3654,7 +3663,11 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i checkpoints_timed bigint - Number of scheduled checkpoints that have been performed + Number of scheduled checkpoints that have been performed. Use of this + field under pg_stat_bgwriter view has been + deprecated, because the field actually shows checkpointer process + activity and is moved to a new view called + pg_stat_checkpointer. @@ -3663,7 +3676,11 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i checkpoints_req bigint - Number of requested checkpoints that have been performed + Number of requested checkpoints that have been performed. Use of this + field under pg_stat_bgwriter view has been + deprecated, because the field actually shows checkpointer process + activity and is moved to a new view called + pg_stat_checkpoint
Re: Introduce a new view for checkpointer related stats
Hi, On 2022-11-23 11:39:43 +0530, Bharath Rupireddy wrote: > On Wed, Nov 23, 2022 at 2:23 AM Andres Freund 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
Re: Introduce a new view for checkpointer related stats
On Wed, Nov 23, 2022 at 2:23 AM Andres Freund 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
Re: Introduce a new view for checkpointer related stats
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
Re: Introduce a new view for checkpointer related stats
On Tue, Nov 22, 2022 at 1:26 PM Drouvot, Bertrand 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. > > + > + The pg_stat_checkpointer view will always have a > + single row, containing global data for the cluster. > + > > 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 From 724d9bee5bc3bc124dd37909878a6450c4fe1019 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy Date: Tue, 22 Nov 2022 12:06:20 + Subject: [PATCH v2] Introduce a new view for checkpointer related stats 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. It is time for us to separate checkpointer stats to its own view called pg_stat_checkpointer. Bump catalog version. --- doc/src/sgml/monitoring.sgml | 110 +- src/backend/catalog/system_views.sql | 18 +-- .../utils/activity/pgstat_checkpointer.c | 1 + src/backend/utils/adt/pgstatfuncs.c | 19 +-- src/include/catalog/pg_proc.dat | 22 ++-- src/include/pgstat.h | 1 + src/test/recovery/t/029_stats_restart.pl | 6 +- src/test/regress/expected/rules.out | 15 +-- src/test/regress/expected/stats.out | 21 +++- src/test/regress/sql/stats.sql| 12 +- 10 files changed, 155 insertions(+), 70 deletions(-) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index e2426f7210..e532c4be08 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -448,6 +448,15 @@ postgres 27093 0.0 0.0 30096 2752 ?Ss 11:34 0:00 postgres: ser + + pg_stat_checkpointerpg_stat_checkpointer + One row only, showing statistics about the + checkpointer process's activity. See + + pg_stat_checkpointer for details. + + + pg_stat_walpg_stat_wal One row only, showing
Re: 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. + + The pg_stat_checkpointer view will always have a + single row, containing global data for the cluster. + 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
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 From 707b3f0e0e7cf55b48a09709b070341d23ad03b8 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy Date: Thu, 17 Nov 2022 12:20:13 + Subject: [PATCH v1] Introduce a new view for checkpointer related stats 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. It is time for us to separate checkpointer stats to its own view called pg_stat_checkpointer. Bump catalog version. --- doc/src/sgml/monitoring.sgml | 108 +- src/backend/catalog/system_views.sql | 18 +-- .../utils/activity/pgstat_checkpointer.c | 1 + src/backend/utils/adt/pgstatfuncs.c | 19 +-- src/include/catalog/pg_proc.dat | 22 ++-- src/include/pgstat.h | 1 + src/test/recovery/t/029_stats_restart.pl | 6 +- src/test/regress/expected/rules.out | 15 +-- src/test/regress/expected/stats.out | 21 +++- src/test/regress/sql/stats.sql| 12 +- 10 files changed, 154 insertions(+), 69 deletions(-) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index e5d622d514..52c3118727 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -448,6 +448,15 @@ postgres 27093 0.0 0.0 30096 2752 ?Ss 11:34 0:00 postgres: ser + + pg_stat_checkpointerpg_stat_checkpointer + One row only, showing statistics about the + checkpointer process's activity. See + + pg_stat_checkpointer for details. + + + pg_stat_walpg_stat_wal One row only, showing statistics about WAL activity. See @@ -3647,97 +3656,138 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i - checkpoints_timed bigint + buffers_clean bigint - Number of scheduled checkpoints that have been performed + Number of buffers written by the background writer - checkpoints_req bigint + maxwritten_clean bigint - Number of requested checkpoints that have been performed + Number of times the background writer stopped a cleaning + scan because it had written too many buffers - checkpoint_write_time double precision + buffers_alloc bigint - Total amount of time that has been spent in the portion of - checkpoint processing where files are written to disk, in milliseconds + Number of buffers allocated - checkpoint_sync_time double precision + stats_reset timestamp with time zone - Total amount of time that has been spent in the portion of - checkpoint processing where files are synchronized to disk, in - milliseconds + Time at which these statistics were last reset + + + + + + + + pg_stat_checkpointer + + + pg_stat_checkpointer + + + The pg_stat_checkpointer view will always have a + single row, containing global data for the cluster. + + + + pg_stat_checkpointer View + + - buffers_checkpoint bigint + Column Type - Number of buffers written during checkpoints + Description + + + + + + + + checkpoints_timed bigint + + + Number of scheduled checkpoints that have been performed - buffers_clean bigint + checkpoints_req bigint - Number of buffers written by the background writer + Number of requested checkpoints that have been performed - maxwritten_clean bigint + checkpoint_write_time double
