Re: Clearing global statistics

2010/1/14 Greg Smith <greg(at)2ndquadrant(dot)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 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.

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 objects such 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 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.

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 Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/