pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: Track block level checksum failures in pg_stat_database
10+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2019-05-01 659e53498c Fix union for pgstat message types
* pgsql: Track block level checksum failures in pg_stat_database
@ 2019-03-09 18:48 Magnus Hagander <magnus@hagander.net>
0 siblings, 1 reply; 10+ messages in thread
From: Magnus Hagander @ 2019-03-09 18:48 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Track block level checksum failures in pg_stat_database
This adds a column that counts how many checksum failures have occurred
on files belonging to a specific database. Both checksum failures
during normal backend processing and those created when a base backup
detects a checksum failure are counted.
Author: Magnus Hagander
Reviewed by: Julien Rouhaud
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/6b9e875f7286d8535bff7955e5aa3602e188e436
Modified Files
--------------
doc/src/sgml/monitoring.sgml | 5 ++++
src/backend/catalog/system_views.sql | 1 +
src/backend/postmaster/pgstat.c | 56 ++++++++++++++++++++++++++++++++++++
src/backend/replication/basebackup.c | 16 +++++++----
src/backend/storage/page/bufpage.c | 3 ++
src/backend/utils/adt/pgstatfuncs.c | 15 ++++++++++
src/include/catalog/catversion.h | 2 +-
src/include/catalog/pg_proc.dat | 4 +++
src/include/pgstat.h | 18 +++++++++++-
src/test/regress/expected/rules.out | 1 +
10 files changed, 114 insertions(+), 7 deletions(-)
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Track block level checksum failures in pg_stat_database
@ 2019-04-30 16:33 Tomas Vondra <tomas.vondra@2ndquadrant.com>
parent: Magnus Hagander <magnus@hagander.net>
0 siblings, 1 reply; 10+ messages in thread
From: Tomas Vondra @ 2019-04-30 16:33 UTC (permalink / raw)
To: Magnus Hagander <magnus@hagander.net>; +Cc: pgsql-committers@lists.postgresql.org
On Sat, Mar 09, 2019 at 06:48:11PM +0000, Magnus Hagander wrote:
>Track block level checksum failures in pg_stat_database
>
>This adds a column that counts how many checksum failures have occurred
>on files belonging to a specific database. Both checksum failures
>during normal backend processing and those created when a base backup
>detects a checksum failure are counted.
>
It seems this commit forgot to add PgStat_MsgChecksumFailure to the
PgStat_Msg union.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Track block level checksum failures in pg_stat_database
@ 2019-04-30 21:41 Julien Rouhaud <rjuju123@gmail.com>
parent: Tomas Vondra <tomas.vondra@2ndquadrant.com>
0 siblings, 1 reply; 10+ messages in thread
From: Julien Rouhaud @ 2019-04-30 21:41 UTC (permalink / raw)
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>; +Cc: Magnus Hagander <magnus@hagander.net>; pgsql-committers@lists.postgresql.org
On Tue, Apr 30, 2019 at 6:33 PM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
>
> On Sat, Mar 09, 2019 at 06:48:11PM +0000, Magnus Hagander wrote:
> >Track block level checksum failures in pg_stat_database
> >
> >This adds a column that counts how many checksum failures have occurred
> >on files belonging to a specific database. Both checksum failures
> >during normal backend processing and those created when a base backup
> >detects a checksum failure are counted.
> >
>
> It seems this commit forgot to add PgStat_MsgChecksumFailure to the
> PgStat_Msg union.
Oops, indeed. That's embarrassing. Trivial fix attached if that helps.
Attachments:
fix_pgstat_msg-v1.diff (text/x-patch, 383B) download
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index fa5dca3b87..c4cdd9e48f 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -569,6 +569,7 @@ typedef union PgStat_Msg
PgStat_MsgFuncpurge msg_funcpurge;
PgStat_MsgRecoveryConflict msg_recoveryconflict;
PgStat_MsgDeadlock msg_deadlock;
+ PgStat_MsgChecksumFailure msg_checksumfailure;
} PgStat_Msg;
* Re: pgsql: Track block level checksum failures in pg_stat_database
@ 2019-04-30 21:56 Tom Lane <tgl@sss.pgh.pa.us>
parent: Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 1 reply; 10+ messages in thread
From: Tom Lane @ 2019-04-30 21:56 UTC (permalink / raw)
To: Julien Rouhaud <rjuju123@gmail.com>; +Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; pgsql-committers@lists.postgresql.org
Julien Rouhaud <rjuju123@gmail.com> writes:
> On Tue, Apr 30, 2019 at 6:33 PM Tomas Vondra
> <tomas.vondra@2ndquadrant.com> wrote:
>> It seems this commit forgot to add PgStat_MsgChecksumFailure to the
>> PgStat_Msg union.
> Oops, indeed. That's embarrassing. Trivial fix attached if that helps.
Seems like this exposes a generic weakness in the way pgstat.c is coded.
I'm inclined to adjust the switch logic in PgstatCollectorMain along
the lines of
- pgstat_recv_inquiry((PgStat_MsgInquiry *) &msg, len);
+ pgstat_recv_inquiry(&msg.msg_inquiry, len);
so that you *couldn't* forget to extend the union, as long as you
followed the existing code's style.
regards, tom lane
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Track block level checksum failures in pg_stat_database
@ 2019-04-30 22:48 Julien Rouhaud <rjuju123@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 10+ messages in thread
From: Julien Rouhaud @ 2019-04-30 22:48 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; pgsql-committers@lists.postgresql.org
On Tue, Apr 30, 2019 at 11:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Julien Rouhaud <rjuju123@gmail.com> writes:
> > On Tue, Apr 30, 2019 at 6:33 PM Tomas Vondra
> > <tomas.vondra@2ndquadrant.com> wrote:
> >> It seems this commit forgot to add PgStat_MsgChecksumFailure to the
> >> PgStat_Msg union.
>
> > Oops, indeed. That's embarrassing. Trivial fix attached if that helps.
>
> Seems like this exposes a generic weakness in the way pgstat.c is coded.
> I'm inclined to adjust the switch logic in PgstatCollectorMain along
> the lines of
>
> - pgstat_recv_inquiry((PgStat_MsgInquiry *) &msg, len);
> + pgstat_recv_inquiry(&msg.msg_inquiry, len);
>
> so that you *couldn't* forget to extend the union, as long as you
> followed the existing code's style.
Right, I was also wondering why there wasn't compile error in such
case. I'll send an updated patch tomorrow, as I usually regret any
code I write after midnight.
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Track block level checksum failures in pg_stat_database
@ 2019-05-01 07:55 Julien Rouhaud <rjuju123@gmail.com>
parent: Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 1 reply; 10+ messages in thread
From: Julien Rouhaud @ 2019-05-01 07:55 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>; Magnus Hagander <magnus@hagander.net>; pgsql-committers@lists.postgresql.org
On Wed, May 1, 2019 at 12:48 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Tue, Apr 30, 2019 at 11:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Julien Rouhaud <rjuju123@gmail.com> writes:
> > > On Tue, Apr 30, 2019 at 6:33 PM Tomas Vondra
> > > <tomas.vondra@2ndquadrant.com> wrote:
> > >> It seems this commit forgot to add PgStat_MsgChecksumFailure to the
> > >> PgStat_Msg union.
> >
> > > Oops, indeed. That's embarrassing. Trivial fix attached if that helps.
> >
> > Seems like this exposes a generic weakness in the way pgstat.c is coded.
> > I'm inclined to adjust the switch logic in PgstatCollectorMain along
> > the lines of
> >
> > - pgstat_recv_inquiry((PgStat_MsgInquiry *) &msg, len);
> > + pgstat_recv_inquiry(&msg.msg_inquiry, len);
> >
> > so that you *couldn't* forget to extend the union, as long as you
> > followed the existing code's style.
>
> I'll send an updated patch tomorrow
Here's an updated version. It turns out that PgStat_MsgTempFile had
also been forgotten and never noticed.
Attachments:
fix_pgstat_msg-v2.diff (text/x-patch, 4.2K) download
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index cdf87bae32..4cfae15d3a 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -4477,80 +4477,83 @@ PgstatCollectorMain(int argc, char *argv[])
break;
case PGSTAT_MTYPE_INQUIRY:
- pgstat_recv_inquiry((PgStat_MsgInquiry *) &msg, len);
+ pgstat_recv_inquiry(&msg.msg_inquiry, len);
break;
case PGSTAT_MTYPE_TABSTAT:
- pgstat_recv_tabstat((PgStat_MsgTabstat *) &msg, len);
+ pgstat_recv_tabstat(&msg.msg_tabstat, len);
break;
case PGSTAT_MTYPE_TABPURGE:
- pgstat_recv_tabpurge((PgStat_MsgTabpurge *) &msg, len);
+ pgstat_recv_tabpurge(&msg.msg_tabpurge, len);
break;
case PGSTAT_MTYPE_DROPDB:
- pgstat_recv_dropdb((PgStat_MsgDropdb *) &msg, len);
+ pgstat_recv_dropdb(&msg.msg_dropdb, len);
break;
case PGSTAT_MTYPE_RESETCOUNTER:
- pgstat_recv_resetcounter((PgStat_MsgResetcounter *) &msg,
- len);
+ pgstat_recv_resetcounter(&msg.msg_resetcounter, len);
break;
case PGSTAT_MTYPE_RESETSHAREDCOUNTER:
pgstat_recv_resetsharedcounter(
- (PgStat_MsgResetsharedcounter *) &msg,
+ &msg.msg_resetsharedcounter,
len);
break;
case PGSTAT_MTYPE_RESETSINGLECOUNTER:
pgstat_recv_resetsinglecounter(
- (PgStat_MsgResetsinglecounter *) &msg,
+ &msg.msg_resetsinglecounter,
len);
break;
case PGSTAT_MTYPE_AUTOVAC_START:
- pgstat_recv_autovac((PgStat_MsgAutovacStart *) &msg, len);
+ pgstat_recv_autovac(&msg.msg_autovacuum_start, len);
break;
case PGSTAT_MTYPE_VACUUM:
- pgstat_recv_vacuum((PgStat_MsgVacuum *) &msg, len);
+ pgstat_recv_vacuum(&msg.msg_vacuum, len);
break;
case PGSTAT_MTYPE_ANALYZE:
- pgstat_recv_analyze((PgStat_MsgAnalyze *) &msg, len);
+ pgstat_recv_analyze(&msg.msg_analyze, len);
break;
case PGSTAT_MTYPE_ARCHIVER:
- pgstat_recv_archiver((PgStat_MsgArchiver *) &msg, len);
+ pgstat_recv_archiver(&msg.msg_archiver, len);
break;
case PGSTAT_MTYPE_BGWRITER:
- pgstat_recv_bgwriter((PgStat_MsgBgWriter *) &msg, len);
+ pgstat_recv_bgwriter(&msg.msg_bgwriter, len);
break;
case PGSTAT_MTYPE_FUNCSTAT:
- pgstat_recv_funcstat((PgStat_MsgFuncstat *) &msg, len);
+ pgstat_recv_funcstat(&msg.msg_funcstat, len);
break;
case PGSTAT_MTYPE_FUNCPURGE:
- pgstat_recv_funcpurge((PgStat_MsgFuncpurge *) &msg, len);
+ pgstat_recv_funcpurge(&msg.msg_funcpurge, len);
break;
case PGSTAT_MTYPE_RECOVERYCONFLICT:
- pgstat_recv_recoveryconflict((PgStat_MsgRecoveryConflict *) &msg, len);
+ pgstat_recv_recoveryconflict(
+ &msg.msg_recoveryconflict,
+ len);
break;
case PGSTAT_MTYPE_DEADLOCK:
- pgstat_recv_deadlock((PgStat_MsgDeadlock *) &msg, len);
+ pgstat_recv_deadlock(&msg.msg_deadlock, len);
break;
case PGSTAT_MTYPE_TEMPFILE:
- pgstat_recv_tempfile((PgStat_MsgTempFile *) &msg, len);
+ pgstat_recv_tempfile(&msg.msg_tempfile, len);
break;
case PGSTAT_MTYPE_CHECKSUMFAILURE:
- pgstat_recv_checksum_failure((PgStat_MsgChecksumFailure *) &msg, len);
+ pgstat_recv_checksum_failure(
+ &msg.msg_checksumfailure,
+ len);
break;
default:
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index fa5dca3b87..9fbc492530 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -560,7 +560,7 @@ typedef union PgStat_Msg
PgStat_MsgResetcounter msg_resetcounter;
PgStat_MsgResetsharedcounter msg_resetsharedcounter;
PgStat_MsgResetsinglecounter msg_resetsinglecounter;
- PgStat_MsgAutovacStart msg_autovacuum;
+ PgStat_MsgAutovacStart msg_autovacuum_start;
PgStat_MsgVacuum msg_vacuum;
PgStat_MsgAnalyze msg_analyze;
PgStat_MsgArchiver msg_archiver;
@@ -569,6 +569,8 @@ typedef union PgStat_Msg
PgStat_MsgFuncpurge msg_funcpurge;
PgStat_MsgRecoveryConflict msg_recoveryconflict;
PgStat_MsgDeadlock msg_deadlock;
+ PgStat_MsgTempFile msg_tempfile;
+ PgStat_MsgChecksumFailure msg_checksumfailure;
} PgStat_Msg;
* Re: pgsql: Track block level checksum failures in pg_stat_database
@ 2019-05-01 10:04 Magnus Hagander <magnus@hagander.net>
parent: Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 1 reply; 10+ messages in thread
From: Magnus Hagander @ 2019-05-01 10:04 UTC (permalink / raw)
To: Julien Rouhaud <rjuju123@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; pgsql-committers@lists.postgresql.org
On Wed, May 1, 2019 at 9:56 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
> On Wed, May 1, 2019 at 12:48 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
> >
> > On Tue, Apr 30, 2019 at 11:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >
> > > Julien Rouhaud <rjuju123@gmail.com> writes:
> > > > On Tue, Apr 30, 2019 at 6:33 PM Tomas Vondra
> > > > <tomas.vondra@2ndquadrant.com> wrote:
> > > >> It seems this commit forgot to add PgStat_MsgChecksumFailure to the
> > > >> PgStat_Msg union.
> > >
> > > > Oops, indeed. That's embarrassing. Trivial fix attached if that
> helps.
> > >
> > > Seems like this exposes a generic weakness in the way pgstat.c is
> coded.
> > > I'm inclined to adjust the switch logic in PgstatCollectorMain along
> > > the lines of
> > >
> > > - pgstat_recv_inquiry((PgStat_MsgInquiry *) &msg,
> len);
> > > + pgstat_recv_inquiry(&msg.msg_inquiry, len);
> > >
> > > so that you *couldn't* forget to extend the union, as long as you
> > > followed the existing code's style.
> >
> > I'll send an updated patch tomorrow
>
> Here's an updated version. It turns out that PgStat_MsgTempFile had
> also been forgotten and never noticed.
>
Looks good to me in general
I don't see the reasoning behind changing the name from msg_autovacuum to
msg_autovacuum_start anywhere, perhaps I missed a part of the discussion?
Was that a change intended to be part of it?
--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/;
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Track block level checksum failures in pg_stat_database
@ 2019-05-01 10:11 Julien Rouhaud <rjuju123@gmail.com>
parent: Magnus Hagander <magnus@hagander.net>
0 siblings, 1 reply; 10+ messages in thread
From: Julien Rouhaud @ 2019-05-01 10:11 UTC (permalink / raw)
To: Magnus Hagander <magnus@hagander.net>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; pgsql-committers@lists.postgresql.org
On Wed, May 1, 2019 at 12:04 PM Magnus Hagander <magnus@hagander.net> wrote:
>
> Looks good to me in general
>
> I don't see the reasoning behind changing the name from msg_autovacuum to msg_autovacuum_start anywhere, perhaps I missed a part of the discussion? Was that a change intended to be part of it?
That wasn't discussed, I should have mentioned it sorry. I just
changed it for consistency while at it, as it seemed a little bit
ambiguous or error prone:
PgStat_MsgAutovacStart -> msg_autovacuum
PgStat_MsgVacuum -> msg_vacuum
As the member of the union weren't used anywhere, I thought it's ok to
rename it now they're used. I'm not strongly attached to this change,
so feel free to discard it.
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Track block level checksum failures in pg_stat_database
@ 2019-05-01 10:34 Magnus Hagander <magnus@hagander.net>
parent: Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 1 reply; 10+ messages in thread
From: Magnus Hagander @ 2019-05-01 10:34 UTC (permalink / raw)
To: Julien Rouhaud <rjuju123@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; pgsql-committers@lists.postgresql.org
On Wed, May 1, 2019 at 12:12 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> On Wed, May 1, 2019 at 12:04 PM Magnus Hagander <magnus@hagander.net>
> wrote:
> >
> > Looks good to me in general
> >
> > I don't see the reasoning behind changing the name from msg_autovacuum
> to msg_autovacuum_start anywhere, perhaps I missed a part of the
> discussion? Was that a change intended to be part of it?
>
> That wasn't discussed, I should have mentioned it sorry. I just
> changed it for consistency while at it, as it seemed a little bit
> ambiguous or error prone:
>
> PgStat_MsgAutovacStart -> msg_autovacuum
> PgStat_MsgVacuum -> msg_vacuum
>
> As the member of the union weren't used anywhere, I thought it's ok to
> rename it now they're used. I'm not strongly attached to this change,
> so feel free to discard it.
>
Yeah, that does make sense. I just wanted to make sure I wasn't missing
some discussion about it.
Pushed, thanks.
--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/;
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Track block level checksum failures in pg_stat_database
@ 2019-05-01 10:36 Julien Rouhaud <rjuju123@gmail.com>
parent: Magnus Hagander <magnus@hagander.net>
0 siblings, 0 replies; 10+ messages in thread
From: Julien Rouhaud @ 2019-05-01 10:36 UTC (permalink / raw)
To: Magnus Hagander <magnus@hagander.net>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Tomas Vondra <tomas.vondra@2ndquadrant.com>; pgsql-committers@lists.postgresql.org
On Wed, May 1, 2019 at 12:35 PM Magnus Hagander <magnus@hagander.net> wrote:
>
> On Wed, May 1, 2019 at 12:12 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>>
>> On Wed, May 1, 2019 at 12:04 PM Magnus Hagander <magnus@hagander.net> wrote:
>> >
>> > Looks good to me in general
>> >
>> > I don't see the reasoning behind changing the name from msg_autovacuum to msg_autovacuum_start anywhere, perhaps I missed a part of the discussion? Was that a change intended to be part of it?
>>
>> That wasn't discussed, I should have mentioned it sorry. I just
>> changed it for consistency while at it, as it seemed a little bit
>> ambiguous or error prone:
>>
>> PgStat_MsgAutovacStart -> msg_autovacuum
>> PgStat_MsgVacuum -> msg_vacuum
>>
>> As the member of the union weren't used anywhere, I thought it's ok to
>> rename it now they're used. I'm not strongly attached to this change,
>> so feel free to discard it.
>
>
> Yeah, that does make sense. I just wanted to make sure I wasn't missing some discussion about it.
ok :)
> Pushed, thanks.
Thanks!
^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2019-05-01 10:36 UTC | newest]
Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-03-09 18:48 pgsql: Track block level checksum failures in pg_stat_database Magnus Hagander <magnus@hagander.net>
2019-04-30 16:33 ` Tomas Vondra <tomas.vondra@2ndquadrant.com>
2019-04-30 21:41 ` Julien Rouhaud <rjuju123@gmail.com>
2019-04-30 21:56 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-04-30 22:48 ` Julien Rouhaud <rjuju123@gmail.com>
2019-05-01 07:55 ` Julien Rouhaud <rjuju123@gmail.com>
2019-05-01 10:04 ` Magnus Hagander <magnus@hagander.net>
2019-05-01 10:11 ` Julien Rouhaud <rjuju123@gmail.com>
2019-05-01 10:34 ` Magnus Hagander <magnus@hagander.net>
2019-05-01 10:36 ` Julien Rouhaud <rjuju123@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox