Re: Clearing global statistics

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/