Обсуждение: Clearing global statistics
Since the pg_stat_bgwriter structure was introduced in 8.3, there's never been any way to reset its statistics back to 0. A week of analyzing data from that every day drove me crazy enough to finally fix this with the attached patch. This implements the TODO item "Allow the clearing of cluster-level statistics", based on previous discussion ending at http://archives.postgresql.org/pgsql-hackers/2009-03/msg00920.php Here's the patch in action: gsmith=# select checkpoints_req,buffers_alloc from pg_stat_bgwriter; checkpoints_req | buffers_alloc -----------------+--------------- 1 | 5 gsmith=# select pg_stat_reset_global(); gsmith=# select checkpoints_req,buffers_alloc from pg_stat_bgwriter; checkpoints_req | buffers_alloc -----------------+--------------- 0 | 0 Patch is complete including docs, it's basically just pg_stat_reset with a different clearing mechanism at the very end. My list of potential questions here are: -Not sure if this should be named pg_stat_rest_global (to match the way these are called "global stats" in the source) or pg_stat_reset_cluster. Picked the former for V1, not attached to that decision at all. Might even make sense to use a name that makes it obvious the pg_stat_bgwriter data is what's targeted. -I create a new stats message type for this, but just reuse the same message payload structure as pg_stats_reset rather than add a new payload structure for no good reason. That's marked with two XXX s in the code as a questionable design decision. I can implement that too if there's some reason it's a good idea I don't know yet. -I grabbed what looked like an appropriate unused OID. I'm never sure if I did that right or not though, it may need to be renumbered. Since this whole patch is basically a cut and paste job of code that was already there, I don't really expect it to need much discussion beyond these minor points; wouldn't have sent it in the middle of an active CommitFest if that weren't the case. -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg@2ndQuadrant.com www.2ndQuadrant.com diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 1f70fd4..2af5696 100644 *** a/doc/src/sgml/monitoring.sgml --- b/doc/src/sgml/monitoring.sgml *************** postgres: <replaceable>user</> <replacea *** 918,923 **** --- 918,932 ---- (requires superuser privileges) </entry> </row> + + <row> + <entry><literal><function>pg_stat_reset_global</function>()</literal></entry> + <entry><type>void</type></entry> + <entry> + Reset the global statistics counters for the database cluster to + zero (requires superuser privileges) + </entry> + </row> </tbody> </tgroup> </table> diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c index 2d4fb77..b124a3e 100644 *** a/src/backend/postmaster/pgstat.c --- b/src/backend/postmaster/pgstat.c *************** static void pgstat_recv_tabstat(PgStat_M *** 269,274 **** --- 269,275 ---- static void pgstat_recv_tabpurge(PgStat_MsgTabpurge *msg, int len); static void pgstat_recv_dropdb(PgStat_MsgDropdb *msg, int len); static void pgstat_recv_resetcounter(PgStat_MsgResetcounter *msg, int len); + static void pgstat_recv_resetglobalcounter(PgStat_MsgResetcounter *msg, int len); static void pgstat_recv_autovac(PgStat_MsgAutovacStart *msg, int len); static void pgstat_recv_vacuum(PgStat_MsgVacuum *msg, int len); static void pgstat_recv_analyze(PgStat_MsgAnalyze *msg, int len); *************** pgstat_reset_counters(void) *** 1150,1155 **** --- 1151,1182 ---- pgstat_send(&msg, sizeof(msg)); } + /* ---------- + * pgstat_reset_global_counters() - + * + * Tell the statistics collector to reset counters for cluster-wide globals. + * ---------- + */ + void + pgstat_reset_global_counters(void) + { + /* XXX Since there's no payload needed, reusing the existing reset counter + structure rather than defining a new one. Is that OK? */ + PgStat_MsgResetcounter msg; + + if (pgStatSock < 0) + return; + + if (!superuser()) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("must be superuser to reset statistics counters"))); + + pgstat_setheader(&msg.m_hdr, PGSTAT_MTYPE_RESETGLOBALCOUNTER); + /* XXX Not actually using this */ + msg.m_databaseid = MyDatabaseId; + pgstat_send(&msg, sizeof(msg)); + } /* ---------- * pgstat_report_autovac() - *************** PgstatCollectorMain(int argc, char *argv *** 2888,2893 **** --- 2915,2926 ---- len); break; + case PGSTAT_MTYPE_RESETGLOBALCOUNTER: + pgstat_recv_resetglobalcounter( + (PgStat_MsgResetcounter *) &msg, + len); + break; + case PGSTAT_MTYPE_AUTOVAC_START: pgstat_recv_autovac((PgStat_MsgAutovacStart *) &msg, len); break; *************** pgstat_recv_resetcounter(PgStat_MsgReset *** 3840,3845 **** --- 3873,3890 ---- } /* ---------- + * pgstat_recv_resetglobalcounter() - + * + * Reset the statistics for the specified cluster. + * ---------- + */ + static void + pgstat_recv_resetglobalcounter(PgStat_MsgResetcounter *msg, int len) + { + memset(&globalStats, 0, sizeof(globalStats)); + } + + /* ---------- * pgstat_recv_autovac() - * * Process an autovacuum signalling message. diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c index fc245d0..70d2bc9 100644 *** a/src/backend/utils/adt/pgstatfuncs.c --- b/src/backend/utils/adt/pgstatfuncs.c *************** extern Datum pg_stat_get_buf_alloc(PG_FU *** 78,83 **** --- 78,84 ---- extern Datum pg_stat_clear_snapshot(PG_FUNCTION_ARGS); extern Datum pg_stat_reset(PG_FUNCTION_ARGS); + extern Datum pg_stat_reset_global(PG_FUNCTION_ARGS); /* Global bgwriter statistics, from bgwriter.c */ extern PgStat_MsgBgWriter bgwriterStats; *************** pg_stat_reset(PG_FUNCTION_ARGS) *** 1108,1110 **** --- 1109,1120 ---- PG_RETURN_VOID(); } + + /* Reset all the cluster-wide global counters */ + Datum + pg_stat_reset_global(PG_FUNCTION_ARGS) + { + pgstat_reset_global_counters(); + + PG_RETURN_VOID(); + } diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h index bcb20aa..e04fd00 100644 *** a/src/include/catalog/pg_proc.h --- b/src/include/catalog/pg_proc.h *************** DATA(insert OID = 2230 ( pg_stat_clear_ *** 3069,3074 **** --- 3069,3076 ---- DESCR("statistics: discard current transaction's statistics snapshot"); DATA(insert OID = 2274 ( pg_stat_reset PGNSP PGUID 12 1 0 0 f f f f f v 0 0 2278 "" _null_ _null_ _null__null_ pg_stat_reset _null_ _null_ _null_ )); DESCR("statistics: reset collected statistics for current database"); + DATA(insert OID = 2275 ( pg_stat_reset_global PGNSP PGUID 12 1 0 0 f f f f f v 0 0 2278 "" _null_ _null_ _null__null_ pg_stat_reset_global _null_ _null_ _null_ )); + DESCR("statistics: reset global collected statistics across cluster"); DATA(insert OID = 1946 ( encode PGNSP PGUID 12 1 0 0 f f f t f i 2 0 25 "17 25" _null_ _null_ _null__null_ binary_encode _null_ _null_ _null_ )); DESCR("convert bytea value into some ascii-only text string"); diff --git a/src/include/pgstat.h b/src/include/pgstat.h index 102507d..c372f36 100644 *** a/src/include/pgstat.h --- b/src/include/pgstat.h *************** typedef enum StatMsgType *** 38,43 **** --- 38,44 ---- PGSTAT_MTYPE_TABPURGE, PGSTAT_MTYPE_DROPDB, PGSTAT_MTYPE_RESETCOUNTER, + PGSTAT_MTYPE_RESETGLOBALCOUNTER, PGSTAT_MTYPE_AUTOVAC_START, PGSTAT_MTYPE_VACUUM, PGSTAT_MTYPE_ANALYZE, *************** extern void pgstat_drop_database(Oid dat *** 633,638 **** --- 634,640 ---- extern void pgstat_clear_snapshot(void); extern void pgstat_reset_counters(void); + extern void pgstat_reset_global_counters(void); extern void pgstat_report_autovac(Oid dboid); extern void pgstat_report_vacuum(Oid tableoid, bool shared, bool scanned_all,
Greg Smith <greg@2ndquadrant.com> wrote: > This implements the TODO item "Allow the > clearing of cluster-level statistics", based on previous discussion > ending at http://archives.postgresql.org/pgsql-hackers/2009-03/msg00920.php > -Not sure if this should be named pg_stat_rest_global (to match the way > these are called "global stats" in the source) or > pg_stat_reset_cluster. Picked the former for V1, not attached to that > decision at all. Might even make sense to use a name that makes it > obvious the pg_stat_bgwriter data is what's targeted. A couple of comments: * We will be able to reset global counters and current database's counters. Do we need to have a method to reset other databases'counters? Or, will pg_stat_reset_global just reset counters of all databases? * Is it useful to have a method to reset counters separately? For example, pg_stat_reset( which text ) which := 'buffers'| 'checkpoints' | 'tables' | 'functions' | ... Regards, --- ITAGAKI Takahiro NTT Open Source Software Center
Itagaki Takahiro wrote: <blockquote cite="mid:20091207094457.950B.52131E4D@oss.ntt.co.jp" type="cite"><pre wrap="">GregSmith <a class="moz-txt-link-rfc2396E" href="mailto:greg@2ndquadrant.com"><greg@2ndquadrant.com></a> wrote: </pre><blockquote type="cite"><pre wrap="">-Not sure if this should be named pg_stat_rest_global (to match the way these are called "global stats" in the source) or pg_stat_reset_cluster. Picked the former for V1, not attached to that decision at all. Might even make sense to use a name that makes it obvious the pg_stat_bgwriter data is what's targeted. </pre></blockquote><pre wrap=""> A couple of comments: * We will be able to reset global counters and current database's counters. Do we need to have a method to reset other databases'counters? Or, will pg_stat_reset_global just reset counters of all databases? * Is it useful to have a method to reset counters separately? For example, pg_stat_reset( which text ) which := 'buffers'| 'checkpoints' | 'tables' | 'functions' | ... </pre></blockquote> The fact that you're asking the question thisway suggests to me I've named this completely wrong. pg_stat_reset_global only resets the bits global to all databases. It doesn't touch any of the database-specific things that pg_stat_reset can handle right now. At the moment,the only global information is what's in pg_stat_bgwriter: buffer statistics and checkpoint stats. I'm thinkingthat I should rename this new function to pg_stat_reset_bgwriter so it's obvious how limited its target is. Usingeither "global" or "cluster" for the name is just going to leave people thinking it acts across a much larger area thanit does.<br /><br /><pre class="moz-signature" cols="72">-- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support <a class="moz-txt-link-abbreviated" href="mailto:greg@2ndQuadrant.com">greg@2ndQuadrant.com</a> <a class="moz-txt-link-abbreviated"href="http://www.2ndQuadrant.com">www.2ndQuadrant.com</a> </pre>
Greg Smith <greg@2ndquadrant.com> wrote: > I'm thinking that I should rename this new function > to pg_stat_reset_bgwriter so it's obvious how limited its target is. I don't think it is a good name because we might have another cluster-level statictics not related with bgwriter in the future. I hope you will suggest "extensible" method when you improve this area. To be honest, I have a plan to add performance statistics counters to postgres. It is not bgwriter's counters, but cluster-level. I'd like to use your infrastructure in my work, too :) Regards, --- ITAGAKI Takahiro NTT Open Source Software Center
Itagaki Takahiro wrote: <blockquote cite="mid:20091207100104.950F.52131E4D@oss.ntt.co.jp" type="cite"><pre wrap="">GregSmith <a class="moz-txt-link-rfc2396E" href="mailto:greg@2ndquadrant.com"><greg@2ndquadrant.com></a> wrote: </pre><blockquote type="cite"><pre wrap="">I'm thinking that I should rename this new function to pg_stat_reset_bgwriter so it's obvious how limited its target is. </pre></blockquote><pre wrap=""> I don't think it is a good name because we might have another cluster-level statictics not related with bgwriter in the future. I hope you will suggest "extensible" method when you improve this area. </pre></blockquote> I follow what you mean now. I'll take a look at allowingpg_stat_reset to act on an input as you suggested, rather than adding more of these UDFs for every time somebodyadds a new area they want to target clearing.<br /><br /><pre class="moz-signature" cols="72">-- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support <a class="moz-txt-link-abbreviated" href="mailto:greg@2ndQuadrant.com">greg@2ndQuadrant.com</a> <a class="moz-txt-link-abbreviated"href="http://www.2ndQuadrant.com">www.2ndQuadrant.com</a> </pre>
2009/12/7 Greg Smith <greg@2ndquadrant.com>: > Itagaki Takahiro wrote: > > Greg Smith <greg@2ndquadrant.com> wrote: > > > > I'm thinking that I should rename this new function > to pg_stat_reset_bgwriter so it's obvious how limited its target is. > > > I don't think it is a good name because we might have another cluster-level > statictics not related with bgwriter in the future. I hope you will suggest > "extensible" method when you improve this area. > > > I follow what you mean now. I'll take a look at allowing pg_stat_reset to act on an input as you suggested, rather thanadding more of these UDFs for every time somebody adds a new area they want to target clearing. I have on my TODO to implement the ability to do stats reset on a single object (say, one table only). Please take this into consideration when you design/name this, so theres no unnecessary overlap :-) Same goes for the stats message itself. -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
On sön, 2009-12-06 at 19:50 -0500, Greg Smith wrote: > The fact that you're asking the question this way suggests to me I've > named this completely wrong. pg_stat_reset_global only resets the > bits > global to all databases. It doesn't touch any of the > database-specific > things that pg_stat_reset can handle right now. At the moment, the > only > global information is what's in pg_stat_bgwriter: buffer statistics > and > checkpoint stats. I'm thinking that I should rename this new > function > to pg_stat_reset_bgwriter so it's obvious how limited its target is. > Using either "global" or "cluster" for the name is just going to > leave > people thinking it acts across a much larger area than it does. The term "shared" is used elsewhere to describe the, well, shared catalogs.
Magnus Hagander wrote:
> I have on my TODO to implement the ability to do stats reset on a
> single object (say, one table only). Please take this into
> consideration when you design/name this, so theres no unnecessary
> overlap :-) Same goes for the stats message itself.
>
The idea suggested upthread was to add this:
pg_stat_reset( which text ) which := 'buffers' | 'checkpoints' | 'tables' | 'functions' | ...
Now, the way the pg_stat_bgwriter tables are computed, it doesn't
actually make sense to separate out clearing the buffers/checkpoints
stats, since one of those values is in both categories:
buffers_checkpoint. They're really all too tightly coupled to break
them apart. So I was thinking of this:
pg_stat_reset( which text ) which := 'bgwriter' | ...
I could convert the patch I've got to be an initial implementation of
this new "pg_stat_reset with a parameter", laying some useful groundwork
in the process too. Then people who want to reset more things can just
re-use that same outline and message passing mechanism, just adding
comparisons for new text and a handler to go with it--not even touching
the catalog again.
This may not mesh well with what you plan though. If pg_stat_reset is
updated to reset stats on an individual table, that could be a second
version that takes in a regclass:
pg_stat_reset('tablename'::regclass)
But that seems like a confusing bit of overloading--I can easily see
people thinking that pg_stat_reset('bgwriter') would be resetting the
stats for a relation named 'bgwriter' rather than what it actually does
if I build it that way.
So, combining with Peter's naming suggestion, I think what I should
build is:
pg_stat_reset_shared( which text ) which := 'bgwriter' | ...
Which satisfies what I'm looking for now, and future patches that need
to reset other shared across the cluster statistics can re-use this
without needing to add a whole new function/stats message. I think that
satisfies the cross-section of planned use cases we're expecting now best.
Any comments before I update my patch to do that?
--
Greg Smith 2ndQuadrant Baltimore, MD
PostgreSQL Training, Services and Support
greg@2ndQuadrant.com www.2ndQuadrant.com
Greg Smith escreveu: > pg_stat_reset( which text ) > which := 'buffers' | 'checkpoints' | 'tables' | 'functions' | ... > What about adding 'all' too? Or the idea is resetting all global counters when we call pg_stat_reset() (without parameters)? -- Euler Taveira de Oliveira http://www.timbira.com/
2010/1/12 Greg Smith <greg@2ndquadrant.com>:
> Magnus Hagander wrote:
>>
>> I have on my TODO to implement the ability to do stats reset on a
>> single object (say, one table only). Please take this into
>> consideration when you design/name this, so theres no unnecessary
>> overlap :-) Same goes for the stats message itself.
>>
>
> The idea suggested upthread was to add this:
>
> pg_stat_reset( which text )
> which := 'buffers' | 'checkpoints' | 'tables' | 'functions' | ...
>
> Now, the way the pg_stat_bgwriter tables are computed, it doesn't actually make sense to separate out clearing the
buffers/checkpointsstats, since one of those values is in both categories: buffers_checkpoint. They're really all too
tightlycoupled to break them apart. So I was thinking of this:
>
> pg_stat_reset( which text )
> which := 'bgwriter' | ...
>
> I could convert the patch I've got to be an initial implementation of this new "pg_stat_reset with a parameter",
layingsome useful groundwork in the process too. Then people who want to reset more things can just re-use that same
outlineand message passing mechanism, just adding comparisons for new text and a handler to go with it--not even
touchingthe catalog again.
>
> This may not mesh well with what you plan though. If pg_stat_reset is updated to reset stats on an individual table,
thatcould be a second version that takes in a regclass:
>
> pg_stat_reset('tablename'::regclass)
>
> But that seems like a confusing bit of overloading--I can easily see people thinking that pg_stat_reset('bgwriter')
wouldbe resetting the stats for a relation named 'bgwriter' rather than what it actually does if I build it that way.
>
> So, combining with Peter's naming suggestion, I think what I should build is:
>
> pg_stat_reset_shared( which text )
> which := 'bgwriter' | ...
>
> Which satisfies what I'm looking for now, and future patches that need to reset other shared across the cluster
statisticscan re-use this without needing to add a whole new function/stats message. I think that satisfies the
cross-sectionof planned use cases we're expecting now best.
>
> Any comments before I update my patch to do that?
Are you planning to get this in for the CF? (Yes, I realize there are
only hours left). This is functionality I'd *really* like to see in
8.5, so I'll be happy to work with you to get that committed inside or
outside CF bounds, but it's easier if it's on there for reviewers ;)
(plus, the "outside cf bounds" really only works *before* the
commitfest :P)
-- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Greg Smith wrote: > Magnus Hagander wrote: >> I have on my TODO to implement the ability to do stats reset on a >> single object (say, one table only). Please take this into >> consideration when you design/name this, so theres no unnecessary >> overlap :-) Same goes for the stats message itself. >> [.........] > > Any comments before I update my patch to do that? > Hello One thing I miss from the statistics you can get via pg_stat_* is information about how long we have been collecting stats (or in other words, when was the last time the stats were reset) Statistics without time period information are unfortunately not very usefull for a DBA :-( Before 8.3, we had the stats_reset_on_server_start parameter and the pg_postmaster_start_time() function. This was an easy way of resetting *all* statistics delivered by pg_stat_* and knowing when this was done. We were able to produce stats with information about sec/hours/days average values in an easy way. I tried to discuss this some time ago but we did not get anywhere, Ref: http://archives.postgresql.org/pgsql-general/2009-07/msg00614.php Maybe this time? :-) Is there any chance of implementing a way of knowing when was the last time statistics delivered via pg_stat_* were reset? regards, - --Rafael Martinez, <r.m.guerrero@usit.uio.no>Center for Information Technology ServicesUniversity of Oslo, Norway PGP Public Key: http://folk.uio.no/rafael/ -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.7 (GNU/Linux) iD8DBQFLTx9cBhuKQurGihQRAnTZAJ9afYGu4UShAha0L6Z3OFyqgJ6SJQCffEow sfFKKoT3ODap6JRpn2I1IfI= =bCqY -----END PGP SIGNATURE-----
Rafael Martinez <r.m.guerrero@usit.uio.no> writes:
> Is there any chance of implementing a way of knowing when was the last
> time statistics delivered via pg_stat_* were reset?
Actually, that brings up a more general question: what's with the
enthusiasm for clearing statistics *at all*? ISTM that's something
you should do only in dire emergencies, like the collector went
haywire and has now got a bunch of garbage numbers. The notion of
resetting subsets of the stats seems even more dubious, because now
you have numbers that aren't mutually comparable. So I fail to
understand why the desire to expend valuable development time on
any of this.
regards, tom lane
Tom Lane wrote:
> Actually, that brings up a more general question: what's with the
> enthusiasm for clearing statistics *at all*? ISTM that's something
> you should do only in dire emergencies, like the collector went
> haywire and has now got a bunch of garbage numbers. The notion of
> resetting subsets of the stats seems even more dubious, because now
> you have numbers that aren't mutually comparable. So I fail to
> understand why the desire to expend valuable development time on
> any of this.
>
When doing checkpoint tuning, the usual thing you start with is by
considering the ratio of time to segment-based checkpoints, along with
the corresponding balance of buffers written by the backends vs. the
checkpoint. When that shows poor behavior, typically because
checkpoint_segments is too low, you change its value and then resume
monitoring at the new setting. Right now, you're still carrying around
the history of the bad period forever though, and every check of the
pg_stat_bgwriter requires manually subtracting the earlier values out.
What people would like to do is reset those after adjusting
checkpoint_segments, and then you can eyeball the proportions directly
instead. That's exactly what the patch does. If I didn't see this
request in the field every month I wouldn't have spent a minute on a
patch to add it.
There was a suggestion that subsets of the data I'm clearing might be
useful to target, which I rejected on the bounds that it made it
possible to get an inconsistent set of results as you're concerned
about. You really need to clear everything that shows up in
pg_stat_bgwriter or not touch it at all. The main use case I'm trying
to support is the person who just made a config change and now wants to do:
select pg_stat_reset();
select pg_stat_reset_shared('bgwriter');
So that all of the stats they're now dealing with are from the same
post-tuning time period. Having numbers that are "mutually comparable"
across the whole system is exactly the reason why this new call is
needed, because there's this one part you just can't touch.
--
Greg Smith 2ndQuadrant Baltimore, MD
PostgreSQL Training, Services and Support
greg@2ndQuadrant.com www.2ndQuadrant.com
Euler Taveira de Oliveira wrote: <blockquote cite="mid:4B4D21E1.3010905@timbira.com" type="cite"><pre wrap="">Greg Smithescreveu: </pre><blockquote type="cite"><pre wrap="">pg_stat_reset( which text ) which := 'buffers' | 'checkpoints'| 'tables' | 'functions' | ... </pre></blockquote><pre wrap="">What about adding 'all' too? Or the idea is resetting all global counters when we call pg_stat_reset() (without parameters)? </pre></blockquote><br /> Once there's more than one piece to clear maybe addingin an 'all' target makes sense. In the context of the update patch I've finished, it just doesn't make sense giventhe code involved.<br /><br /><pre class="moz-signature" cols="72">-- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support <a class="moz-txt-link-abbreviated" href="mailto:greg@2ndQuadrant.com">greg@2ndQuadrant.com</a> <a class="moz-txt-link-abbreviated"href="http://www.2ndQuadrant.com">www.2ndQuadrant.com</a> </pre>
2010/1/14 Tom Lane <tgl@sss.pgh.pa.us>: > Rafael Martinez <r.m.guerrero@usit.uio.no> writes: >> Is there any chance of implementing a way of knowing when was the last >> time statistics delivered via pg_stat_* were reset? > > Actually, that brings up a more general question: what's with the > enthusiasm for clearing statistics *at all*? ISTM that's something > you should do only in dire emergencies, like the collector went > haywire and has now got a bunch of garbage numbers. The notion of > resetting subsets of the stats seems even more dubious, because now > you have numbers that aren't mutually comparable. So I fail to > understand why the desire to expend valuable development time on > any of this. s/collector/application/ and you've got one reason. Example, that I hit the other day. Examining pg_stat_user_functions shows one function taking much longer than you'd expect. Called about 6 million times, total time about 7 days spent. Reason turned out to be a missing index. Without clearing the stats, it'll take a *long* time before the average goes down enough to make it possible to use the simple SELECT self_time/calls FROM pg_stat_user_functions WHERE... to monitor. Sure, if you have a system that graphs it, it'll update properly, but for the quick manual checks, that view suddenly becomes a lot less ueful. -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Rafael Martinez wrote:
> One thing I miss from the statistics you can get via pg_stat_* is
> information about how long we have been collecting stats (or in other
> words, when was the last time the stats were reset)
>
I've considered adding this for the same reasons you're asking about it,
but am not happy with the trade-offs involved. The problem is that you
have to presume the server was running the entirety of the time since
stats were reset for that data to be useful. So unless people are in
that situation, they're going to get data that may not represent what
they think it does. Realistically, if you want a timestamp that always
means something useful you have to rest the stats at every server start,
which leads us to:
> Before 8.3, we had the stats_reset_on_server_start parameter and the
> pg_postmaster_start_time() function. This was an easy way of resetting
> *all* statistics delivered by pg_stat_* and knowing when this was done.
> We were able to produce stats with information about sec/hours/days
> average values in an easy way.
>
With this new feature I'm submitting, you can adjust your database
startup scripts to make this happen again. Start the server,
immediately loop over every database and call pg_stat_reset on them all,
and call pg_stat_reset_shared('bgwriter'). Now you've got completely
cleared stats that are within a second or two of
pg_postmaster_start_time(), should be close enough to most purposes.
Theoretically we could automate that better, but I've found it hard to
justify working on given that it's not that difficult to handle outside
of the database once the individual pieces are exposed.
--
Greg Smith 2ndQuadrant Baltimore, MD
PostgreSQL Training, Services and Support
greg@2ndQuadrant.com www.2ndQuadrant.com
Greg Smith <greg@2ndquadrant.com> writes:
> Tom Lane wrote:
>> Actually, that brings up a more general question: what's with the
>> enthusiasm for clearing statistics *at all*?
> ... Right now, you're still carrying around
> the history of the bad period forever though, and every check of the
> pg_stat_bgwriter requires manually subtracting the earlier values out.
Seems like a more appropriate solution would be to make it easier to do
that subtraction, ie, make it easier to capture the values at a given
time point and then get deltas from there. It's more general (you could
have multiple saved sets of values), and doesn't require superuser
permissions to do, and doesn't have the same potential for
damn-I-wish-I-hadn't-done-that moments.
regards, tom lane
Tom Lane wrote: > Seems like a more appropriate solution would be to make it easier to do > that subtraction, ie, make it easier to capture the values at a given > time point and then get deltas from there. It's more general (you could > have multiple saved sets of values), and doesn't require superuser > permissions to do, and doesn't have the same potential for > damn-I-wish-I-hadn't-done-that moments. > You can make the same argument about the existing pg_stat_reset mechanism. I would love to completely rework the stats infrastructure so that it's easier to capture values with timestamps, compute diffs, and do trending. However, I'm not sure the database itself is necessarily the best place to do that at anyway. People who know what they're doing are already handling this exact job using external tools that grab regular snapshots for that purpose, so why try to duplicate that work? I'm trying to triage here, to scrub off the worst of the common problems. I would never claim this is the perfect direction to follow forever. There are a number of people who consider the inability to reset the pg_stat_bgwriter stats in any way a bug that's gone unfixed for two versions now. Your larger point that this style of implementation is not ideal as a long-term way to manage statistics I would completely agree with, I just don't have the time to spend on a major rewrite to improve that. What I can offer is a fix for the most common issue I get complaints about, in the form of a tool much more likely to be used correctly by people who go looking for it than misused IMHO. -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg@2ndQuadrant.com www.2ndQuadrant.com
Greg Smith wrote:
>
>> Before 8.3, we had the stats_reset_on_server_start parameter and
>> the pg_postmaster_start_time() function. This was an easy way of
>> resetting *all* statistics delivered by pg_stat_* and knowing when
>> this was done. We were able to produce stats with information about
>> sec/hours/days average values in an easy way.
>>
>
> With this new feature I'm submitting, you can adjust your database
> startup scripts to make this happen again. Start the server,
> immediately loop over every database and call pg_stat_reset on them
> all, and call pg_stat_reset_shared('bgwriter'). Now you've got
> completely cleared stats that are within a second or two of
> pg_postmaster_start_time(), should be close enough to most purposes.
> Theoretically we could automate that better, but I've found it hard
> to justify working on given that it's not that difficult to handle
> outside of the database once the individual pieces are exposed.
>
Great, this is good enough and we get what we need. Thanks :-)
regards
-- Rafael Martinez, <r.m.guerrero@usit.uio.no>Center for Information Technology ServicesUniversity of Oslo, Norway
PGP Public Key: http://folk.uio.no/rafael/
Itagaki Takahiro wrote:
> To be honest, I have a plan to add performance statistics counters to
> postgres. It is not bgwriter's counters, but cluster-level. I'd like
> to use your infrastructure in my work, too :)
>
Attached patch provides just that. It still works basically the same as
my earlier version, except you pass it a name of what you want to reset,
and if you don't give it the only valid one right now ('bgwriter') it
rejects it (for now):
gsmith=# select checkpoints_req,buffers_alloc from pg_stat_bgwriter;
checkpoints_req | buffers_alloc
-----------------+---------------
4 | 129
(1 row)
gsmith=# select pg_stat_reset_shared('bgwriter');
pg_stat_reset_shared
----------------------
(1 row)
gsmith=# select checkpoints_req,buffers_alloc from pg_stat_bgwriter;
checkpoints_req | buffers_alloc
-----------------+---------------
0 | 7
(1 row)
gsmith=# select pg_stat_reset_shared('rubbish');
ERROR: Unrecognized reset target
I turn the input text into an enum choice as part of composing the
message to the stats collector. If you wanted to add some other shared
cluster-wide reset capabilities into there you could re-use most of this
infrastructure. Just add an extra enum value, map the text into that
enum, and write the actual handler that does the reset work. Should be
able to reuse the same new message type and external UI I implemented
for this specific clearing feature.
I didn't see any interaction to be concerned about here with Magnus's
suggestion he wanted to target stats reset on objects such as a single
table at some point.
The main coding choice I wasn't really sure about is how I flag the
error case where you pass bad in. I do that validation and throw
ERRCODE_SYNTAX_ERROR before composing the message to the stats
collector. Didn't know if I should create a whole new error code just
for this specific case or if reusing another error code was more
appropriate. Also, I didn't actually have the collector process itself
validate the data at all, it just quietly ignores bad messages on the
presumption everything is already being checked during message creation.
That seems consistent with the other code here--the other message
handlers only seem to throw errors when something really terrible
happens, not when they just don't find something useful to do.
--
Greg Smith 2ndQuadrant Baltimore, MD
PostgreSQL Training, Services and Support
greg@2ndQuadrant.com www.2ndQuadrant.com
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 1f70fd4..b630bc8 100644
*** a/doc/src/sgml/monitoring.sgml
--- b/doc/src/sgml/monitoring.sgml
*************** postgres: <replaceable>user</> <replacea
*** 918,923 ****
--- 918,934 ----
(requires superuser privileges)
</entry>
</row>
+
+ <row>
+ <entry><literal><function>pg_stat_reset_shared</function>()</literal></entry>
+ <entry><type>text</type></entry>
+ <entry>
+ Reset some of the shared statistics counters for the database cluster to
+ zero (requires superuser privileges). Calling
+ <literal>pg_stat_reset_shared('bgwriter')</> will zero all the values shown by
+ <structname>pg_stat_bgwriter</>.
+ </entry>
+ </row>
</tbody>
</tgroup>
</table>
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 4fd2abf..7335a50 100644
*** a/src/backend/postmaster/pgstat.c
--- b/src/backend/postmaster/pgstat.c
*************** static void pgstat_recv_tabstat(PgStat_M
*** 270,275 ****
--- 270,276 ----
static void pgstat_recv_tabpurge(PgStat_MsgTabpurge *msg, int len);
static void pgstat_recv_dropdb(PgStat_MsgDropdb *msg, int len);
static void pgstat_recv_resetcounter(PgStat_MsgResetcounter *msg, int len);
+ static void pgstat_recv_resetsharedcounter(PgStat_MsgResetsharedcounter *msg, int len);
static void pgstat_recv_autovac(PgStat_MsgAutovacStart *msg, int len);
static void pgstat_recv_vacuum(PgStat_MsgVacuum *msg, int len);
static void pgstat_recv_analyze(PgStat_MsgAnalyze *msg, int len);
*************** pgstat_reset_counters(void)
*** 1153,1158 ****
--- 1154,1190 ----
pgstat_send(&msg, sizeof(msg));
}
+ /* ----------
+ * pgstat_reset_shared_counters() -
+ *
+ * Tell the statistics collector to reset cluster-wide shared counters.
+ * ----------
+ */
+ void
+ pgstat_reset_shared_counters(const char *target)
+ {
+ PgStat_MsgResetsharedcounter msg;
+
+ if (pgStatSock < 0)
+ return;
+
+ if (!superuser())
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("must be superuser to reset statistics counters")));
+
+ if (strcmp(target, "bgwriter") == 0)
+ msg.m_resettarget = RESET_BGWRITER;
+ else
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("Unrecognized reset target")));
+ }
+
+ pgstat_setheader(&msg.m_hdr, PGSTAT_MTYPE_RESETSHAREDCOUNTER);
+ pgstat_send(&msg, sizeof(msg));
+ }
/* ----------
* pgstat_report_autovac() -
*************** PgstatCollectorMain(int argc, char *argv
*** 2915,2920 ****
--- 2947,2958 ----
len);
break;
+ case PGSTAT_MTYPE_RESETSHAREDCOUNTER:
+ pgstat_recv_resetsharedcounter(
+ (PgStat_MsgResetsharedcounter *) &msg,
+ len);
+ break;
+
case PGSTAT_MTYPE_AUTOVAC_START:
pgstat_recv_autovac((PgStat_MsgAutovacStart *) &msg, len);
break;
*************** pgstat_recv_resetcounter(PgStat_MsgReset
*** 3869,3874 ****
--- 3907,3930 ----
}
/* ----------
+ * pgstat_recv_resetshared() -
+ *
+ * Reset some shared statistics of the cluster.
+ * ----------
+ */
+ static void
+ pgstat_recv_resetsharedcounter(PgStat_MsgResetsharedcounter *msg, int len)
+ {
+ if (msg->m_resettarget==RESET_BGWRITER)
+ {
+ /* Reset the global background writer statistics for the cluster. */
+ memset(&globalStats, 0, sizeof(globalStats));
+ }
+ /* Presumably the sender of this message validated the target, don't
+ complain here if it's not valid */
+ }
+
+ /* ----------
* pgstat_recv_autovac() -
*
* Process an autovacuum signalling message.
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index ab741ca..e5bc95a 100644
*** a/src/backend/utils/adt/pgstatfuncs.c
--- b/src/backend/utils/adt/pgstatfuncs.c
*************** extern Datum pg_stat_get_buf_alloc(PG_FU
*** 78,83 ****
--- 78,84 ----
extern Datum pg_stat_clear_snapshot(PG_FUNCTION_ARGS);
extern Datum pg_stat_reset(PG_FUNCTION_ARGS);
+ extern Datum pg_stat_reset_shared(PG_FUNCTION_ARGS);
/* Global bgwriter statistics, from bgwriter.c */
extern PgStat_MsgBgWriter bgwriterStats;
*************** pg_stat_reset(PG_FUNCTION_ARGS)
*** 1108,1110 ****
--- 1109,1123 ----
PG_RETURN_VOID();
}
+
+ /* Reset some shared cluster-wide counters */
+ Datum
+ pg_stat_reset_shared(PG_FUNCTION_ARGS)
+ {
+ char *target;
+ target = TextDatumGetCString(PG_GETARG_DATUM(0));
+
+ pgstat_reset_shared_counters(target);
+
+ PG_RETURN_VOID();
+ }
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index a8efed9..87072fc 100644
*** a/src/include/catalog/pg_proc.h
--- b/src/include/catalog/pg_proc.h
*************** DATA(insert OID = 2230 ( pg_stat_clear_
*** 3071,3076 ****
--- 3071,3078 ----
DESCR("statistics: discard current transaction's statistics snapshot");
DATA(insert OID = 2274 ( pg_stat_reset PGNSP PGUID 12 1 0 0 f f f f f v 0 0 2278 "" _null_ _null_
_null__null_ pg_stat_reset _null_ _null_ _null_ ));
DESCR("statistics: reset collected statistics for current database");
+ DATA(insert OID = 3775 ( pg_stat_reset_shared PGNSP PGUID 12 1 0 0 f f f f f v 1 0 2278 "25" _null_
_null__null_ _null_ pg_stat_reset_shared _null_ _null_ _null_ ));
+ DESCR("statistics: reset collected statistics shared across the cluster");
DATA(insert OID = 1946 ( encode PGNSP PGUID 12 1 0 0 f f f t f i 2 0 25 "17 25" _null_ _null_
_null__null_ binary_encode _null_ _null_ _null_ ));
DESCR("convert bytea value into some ascii-only text string");
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 3cc334c..c3de665 100644
*** a/src/include/pgstat.h
--- b/src/include/pgstat.h
*************** typedef enum StatMsgType
*** 38,43 ****
--- 38,44 ----
PGSTAT_MTYPE_TABPURGE,
PGSTAT_MTYPE_DROPDB,
PGSTAT_MTYPE_RESETCOUNTER,
+ PGSTAT_MTYPE_RESETSHAREDCOUNTER,
PGSTAT_MTYPE_AUTOVAC_START,
PGSTAT_MTYPE_VACUUM,
PGSTAT_MTYPE_ANALYZE,
*************** typedef struct PgStat_TableCounts
*** 93,98 ****
--- 94,105 ----
PgStat_Counter t_blocks_hit;
} PgStat_TableCounts;
+ /* Possible targets for resetting cluster-wide shared values */
+ typedef enum PgStat_Shared_Reset_Target
+ {
+ RESET_BGWRITER
+ } PgStat_Shared_Reset_Target;
+
/* ------------------------------------------------------------
* Structures kept in backend local memory while accumulating counts
*************** typedef struct PgStat_MsgResetcounter
*** 260,265 ****
--- 267,282 ----
Oid m_databaseid;
} PgStat_MsgResetcounter;
+ /* ----------
+ * PgStat_MsgResetsharedcounter Sent by the backend to tell the collector
+ * to reset a shared counter
+ * ----------
+ */
+ typedef struct PgStat_MsgResetsharedcounter
+ {
+ PgStat_MsgHdr m_hdr;
+ PgStat_Shared_Reset_Target m_resettarget;
+ } PgStat_MsgResetsharedcounter;
/* ----------
* PgStat_MsgAutovacStart Sent by the autovacuum daemon to signal
*************** typedef union PgStat_Msg
*** 414,419 ****
--- 431,437 ----
PgStat_MsgTabpurge msg_tabpurge;
PgStat_MsgDropdb msg_dropdb;
PgStat_MsgResetcounter msg_resetcounter;
+ PgStat_MsgResetsharedcounter msg_resetsharedcounter;
PgStat_MsgAutovacStart msg_autovacuum;
PgStat_MsgVacuum msg_vacuum;
PgStat_MsgAnalyze msg_analyze;
*************** extern void pgstat_drop_database(Oid dat
*** 635,640 ****
--- 653,659 ----
extern void pgstat_clear_snapshot(void);
extern void pgstat_reset_counters(void);
+ extern void pgstat_reset_shared_counters(const char *);
extern void pgstat_report_autovac(Oid dboid);
extern void pgstat_report_vacuum(Oid tableoid, bool shared, bool adopt_counts,
On Thu, Jan 14, 2010 at 1:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Greg Smith <greg@2ndquadrant.com> writes: >> Tom Lane wrote: >>> Actually, that brings up a more general question: what's with the >>> enthusiasm for clearing statistics *at all*? > >> ... Right now, you're still carrying around >> the history of the bad period forever though, and every check of the >> pg_stat_bgwriter requires manually subtracting the earlier values out. > > Seems like a more appropriate solution would be to make it easier to do > that subtraction, ie, make it easier to capture the values at a given > time point and then get deltas from there. It's more general (you could > have multiple saved sets of values), and doesn't require superuser > permissions to do, and doesn't have the same potential for > damn-I-wish-I-hadn't-done-that moments. True, but it's also more complicated to use. Most systems I'm familiar with[1] that have performance counters just provide an option to clear them. Despite the disadvantages you cite, it seems to be fairly useful in practice; anyway, I have found it so. ...Robert [1] The other design I've seen is a system that automatically resets, say, once a day. It retains the statistics for the 24-hour period between the most recent two resets, and the statistics for the partial period following the last reset. But that doesn't seem appropriate for PostgreSQL....
2010/1/14 Greg Smith <greg@2ndquadrant.com>:
> Itagaki Takahiro wrote:
>>
>> To be honest, I have a plan to add performance statistics counters to
>> postgres. It is not bgwriter's counters, but cluster-level. I'd like
>> to use your infrastructure in my work, too :)
>>
>
> Attached patch provides just that. It still works basically the same as my earlier version, except you pass it a name
ofwhat you want to reset, and if you don't give it the only valid one right now ('bgwriter') it rejects it (for now):
>
> gsmith=# select checkpoints_req,buffers_alloc from pg_stat_bgwriter;
> checkpoints_req | buffers_alloc
> -----------------+---------------
> 4 | 129
> (1 row)
>
> gsmith=# select pg_stat_reset_shared('bgwriter');
> pg_stat_reset_shared
> ----------------------
>
> (1 row)
>
> gsmith=# select checkpoints_req,buffers_alloc from pg_stat_bgwriter;
> checkpoints_req | buffers_alloc
> -----------------+---------------
> 0 | 7
> (1 row)
>
> gsmith=# select pg_stat_reset_shared('rubbish');
> ERROR: Unrecognized reset target
>
> I turn the input text into an enum choice as part of composing the message to the stats collector. If you wanted to
addsome other shared cluster-wide reset capabilities into there you could re-use most of this infrastructure. Just add
anextra enum value, map the text into that enum, and write the actual handler that does the reset work. Should be able
toreuse the same new message type and external UI I implemented for this specific clearing feature.
Yeah, that seems fine.
> I didn't see any interaction to be concerned about here with Magnus's suggestion he wanted to target stats reset on
objectssuch as a single table at some point.
Agreed.
> The main coding choice I wasn't really sure about is how I flag the error case where you pass bad in. I do that
validationand throw ERRCODE_SYNTAX_ERROR before composing the message to the stats collector. Didn't know if I should
createa whole new error code just for this specific case or if reusing another error code was more appropriate. Also, I
didn'tactually have the collector process itself validate the data at all, it just quietly ignores bad messages on the
presumptioneverything is already being checked during message creation. That seems consistent with the other code
here--theother message handlers only seem to throw errors when something really terrible happens, not when they just
don'tfind something useful to do.
Creating a whole new error code seems completely wrong.
ERRCODE_SYNTAX_ERROR seems fine to me. As does the not checking the
other options.
I looked the patch over, and I only really see one thing to comment on which is:
+ if (strcmp(target, "bgwriter") == 0)
+ msg.m_resettarget = RESET_BGWRITER;
+ else
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("Unrecognized reset target")));
+ }
Maybe this should be "Unrecognized reset target: %s", target, and also
a errhint() saying which targets are allowed. Thoughts?
As for the discussion about if this is useful or not, I definitely
think it is. Yes, there are certainly general improvements that could
be done to the collection mechanism to make some things easier, but
that doesn't mean we shouldn't make the current solution more useful
until we (potentially) have it.
-- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Magnus Hagander wrote: > Maybe this should be "Unrecognized reset target: %s", target, and also > a errhint() saying which targets are allowed. Thoughts? > That seems reasonable. The other thing I realized is that I forgot to add the new function to the right place in doc/src/sgml/func.sgml : <indexterm> <primary>pg_stat_reset_shared</primary> </indexterm> I can send an updated patch with both of these things fixed tomorrow. Given that we're talking 5 lines of change here, if it's easier for you to just patch a working copy you've already started on that's fine too. -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg@2ndQuadrant.com www.2ndQuadrant.com
On Mon, Jan 18, 2010 at 00:52, Greg Smith <greg@2ndquadrant.com> wrote: > Magnus Hagander wrote: >> >> Maybe this should be "Unrecognized reset target: %s", target, and also >> a errhint() saying which targets are allowed. Thoughts? >> > > That seems reasonable. The other thing I realized is that I forgot to add > the new function to the right place in doc/src/sgml/func.sgml : > > <indexterm> > <primary>pg_stat_reset_shared</primary> > </indexterm> > > I can send an updated patch with both of these things fixed tomorrow. Given > that we're talking 5 lines of change here, if it's easier for you to just > patch a working copy you've already started on that's fine too. I can deal with that, I just wanted to make sure we're in agreement. -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
On Mon, Jan 18, 2010 at 11:02, Magnus Hagander <magnus@hagander.net> wrote: > On Mon, Jan 18, 2010 at 00:52, Greg Smith <greg@2ndquadrant.com> wrote: >> Magnus Hagander wrote: >>> >>> Maybe this should be "Unrecognized reset target: %s", target, and also >>> a errhint() saying which targets are allowed. Thoughts? >>> >> >> That seems reasonable. The other thing I realized is that I forgot to add >> the new function to the right place in doc/src/sgml/func.sgml : >> >> <indexterm> >> <primary>pg_stat_reset_shared</primary> >> </indexterm> >> >> I can send an updated patch with both of these things fixed tomorrow. Given >> that we're talking 5 lines of change here, if it's easier for you to just >> patch a working copy you've already started on that's fine too. > > I can deal with that, I just wanted to make sure we're in agreement. Applied with agreed upon change and some minor styling/formatting changes. The mention you have about documentation above is incorrect - the other pg_stat_xyz functions aren't listed there, so neither should this one. At least from AFAICT ;) -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/