Обсуждение: Resetting a single statistics counter
In the spirit of finishing off small patches, attached is the one that implements pg_stat_reset_single(), to be able to reset stats for a single table or function. I kind of thought it would be included in the patch from Greg Smith for shared counters so I put it aside, but I guess I misunderstood him there. Anyway, I polished off the final part, and here it is. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Вложения
Magnus Hagander <magnus@hagander.net> writes:
> In the spirit of finishing off small patches, attached is the one that
> implements pg_stat_reset_single(), to be able to reset stats for a
> single table or function. I kind of thought it would be included in
> the patch from Greg Smith for shared counters so I put it aside, but I
> guess I misunderstood him there. Anyway, I polished off the final
> part, and here it is.
This is bogus; it assumes tables and functions will not have the same
OIDs.
regards, tom lane
2010/1/24 Tom Lane <tgl@sss.pgh.pa.us>: > Magnus Hagander <magnus@hagander.net> writes: >> In the spirit of finishing off small patches, attached is the one that >> implements pg_stat_reset_single(), to be able to reset stats for a >> single table or function. I kind of thought it would be included in >> the patch from Greg Smith for shared counters so I put it aside, but I >> guess I misunderstood him there. Anyway, I polished off the final >> part, and here it is. > > This is bogus; it assumes tables and functions will not have the same > OIDs. Gah... *faceinpalms* Off to make it two separate functions.. (seems much more user-friendly than a single function with an extra argument, IMHO) -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
On Sun, 2010-01-24 at 18:25 +0100, Magnus Hagander wrote: > 2010/1/24 Tom Lane <tgl@sss.pgh.pa.us>: > > Magnus Hagander <magnus@hagander.net> writes: > >> In the spirit of finishing off small patches, attached is the one that > >> implements pg_stat_reset_single(), to be able to reset stats for a > >> single table or function. I kind of thought it would be included in > >> the patch from Greg Smith for shared counters so I put it aside, but I > >> guess I misunderstood him there. Anyway, I polished off the final > >> part, and here it is. > > > > This is bogus; it assumes tables and functions will not have the same > > OIDs. > > Gah... *faceinpalms* > > Off to make it two separate functions.. (seems much more user-friendly > than a single function with an extra argument, IMHO) And a much better name also :-) -- Simon Riggs www.2ndQuadrant.com
2010/1/24 Magnus Hagander <magnus@hagander.net>: > 2010/1/24 Tom Lane <tgl@sss.pgh.pa.us>: >> Magnus Hagander <magnus@hagander.net> writes: >>> In the spirit of finishing off small patches, attached is the one that >>> implements pg_stat_reset_single(), to be able to reset stats for a >>> single table or function. I kind of thought it would be included in >>> the patch from Greg Smith for shared counters so I put it aside, but I >>> guess I misunderstood him there. Anyway, I polished off the final >>> part, and here it is. >> >> This is bogus; it assumes tables and functions will not have the same >> OIDs. > > Gah... *faceinpalms* > > Off to make it two separate functions.. (seems much more user-friendly > than a single function with an extra argument, IMHO) Here goes. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Вложения
Magnus Hagander <magnus@hagander.net> writes:
> Here goes.
Looks much saner. One minor stylistic gripe:
+Datum
+pg_stat_reset_single_table(PG_FUNCTION_ARGS)
+{
+ pgstat_reset_single_counter(PG_GETARG_OID(0), RESET_TABLE);
+
+ PG_RETURN_VOID();
+}
I don't like sticking PG_GETARG calls inline in the body of a V1-protocol
function, even in trivial cases like this. I think better style is
Oid taboid = PG_GETARG_OID(0);
pgstat_reset_single_counter(taboid, RESET_TABLE);
This approach associates a clear name and type with each argument,
thereby helping to buy back some of the readability we lose by not
being able to use regular C function declarations. When we designed
the V1 call protocol, I had hoped we might someday have scripts that
would crosscheck such declarations against the pg_proc contents, and
I still haven't entirely given up that idea ...
regards, tom lane
Magnus Hagander escreveu:
>> Off to make it two separate functions.. (seems much more user-friendly
>> than a single function with an extra argument, IMHO)
>
+1. But as Simon said _single_ is too ugly. What about
pg_stat_reset_user_{function,relation}?
Another thing that is not a problem of your patch but it needs to be fixed is
that resetting functions remove the line from pg_stat_user_functions; that a
different behavior from other pg_stat_user_* functions.
-- Euler Taveira de Oliveira http://www.timbira.com/
Euler Taveira de Oliveira <euler@timbira.com> writes:
> Magnus Hagander escreveu:
>> Off to make it two separate functions.. (seems much more user-friendly
>> than a single function with an extra argument, IMHO)
> +1. But as Simon said _single_ is too ugly. What about
> pg_stat_reset_user_{function,relation}?
That implies that the operations wouldn't work against system tables;
which they do. I think a bigger problem is that "reset_single_table"
seems like it might be talking about something like a TRUNCATE, ie,
it's not clear that it means to reset counters rather than data.
The pg_stat_ prefix is some help but not enough IMO. So I suggest
pg_stat_reset_table_counters and pg_stat_reset_function_counters.
(BTW, a similar complaint could be made about the previously committed
patch: reset shared what?)
regards, tom lane
2010/1/24 Tom Lane <tgl@sss.pgh.pa.us>:
> Euler Taveira de Oliveira <euler@timbira.com> writes:
>> Magnus Hagander escreveu:
>>> Off to make it two separate functions.. (seems much more user-friendly
>>> than a single function with an extra argument, IMHO)
>
>> +1. But as Simon said _single_ is too ugly. What about
>> pg_stat_reset_user_{function,relation}?
>
> That implies that the operations wouldn't work against system tables;
> which they do. I think a bigger problem is that "reset_single_table"
> seems like it might be talking about something like a TRUNCATE, ie,
> it's not clear that it means to reset counters rather than data.
> The pg_stat_ prefix is some help but not enough IMO. So I suggest
> pg_stat_reset_table_counters and pg_stat_reset_function_counters.
Doesn't the pg_stat_ part already say this?
> (BTW, a similar complaint could be made about the previously committed
> patch: reset shared what?)
Well, it could also be made about the original pg_stat_reset()
function - reset what?
-- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
> 2010/1/24 Tom Lane <tgl@sss.pgh.pa.us>:
>> The pg_stat_ prefix is some help but not enough IMO. �So I suggest
>> pg_stat_reset_table_counters and pg_stat_reset_function_counters.
> Doesn't the pg_stat_ part already say this?
My objection is that "reset_table" sounds like something you do to a
table, not something you do to stats. No, I don't think the prefix is
enough to clarify that.
>> (BTW, a similar complaint could be made about the previously committed
>> patch: reset shared what?)
> Well, it could also be made about the original pg_stat_reset()
> function - reset what?
In that case, there's nothing but the "stat" to suggest what gets
reset, so I think it's less likely to be misleading than the current
proposals. But if we'd been designing all of these at once, yeah,
I'd have argued for a more verbose name for that one too.
regards, tom lane
2010/1/24 Tom Lane <tgl@sss.pgh.pa.us>: > Magnus Hagander <magnus@hagander.net> writes: >> 2010/1/24 Tom Lane <tgl@sss.pgh.pa.us>: >>> The pg_stat_ prefix is some help but not enough IMO. So I suggest >>> pg_stat_reset_table_counters and pg_stat_reset_function_counters. > >> Doesn't the pg_stat_ part already say this? > > My objection is that "reset_table" sounds like something you do to a > table, not something you do to stats. No, I don't think the prefix is > enough to clarify that. Fair enough, I'll just add the _counters to all three functions then. >>> (BTW, a similar complaint could be made about the previously committed >>> patch: reset shared what?) > >> Well, it could also be made about the original pg_stat_reset() >> function - reset what? > > In that case, there's nothing but the "stat" to suggest what gets > reset, so I think it's less likely to be misleading than the current > proposals. But if we'd been designing all of these at once, yeah, > I'd have argued for a more verbose name for that one too. Ok. -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Tom Lane escreveu:
> That implies that the operations wouldn't work against system tables;
> which they do. I think a bigger problem is that "reset_single_table"
> seems like it might be talking about something like a TRUNCATE, ie,
> it's not clear that it means to reset counters rather than data.
> The pg_stat_ prefix is some help but not enough IMO. So I suggest
> pg_stat_reset_table_counters and pg_stat_reset_function_counters.
>
Sure, much better. +1.
> (BTW, a similar complaint could be made about the previously committed
> patch: reset shared what?)
>
BTW, what about that idea to overload pg_stat_reset()? The
pg_stat_reset_shared should be renamed to pg_stat_reset('foo') [1] where foo
is the class of objects that it is resetting. pg_stat_reset is not a so
suggestive name but that's one we already have; besides, it will be intuitive
for users.
[1] http://archives.postgresql.org/pgsql-hackers/2010-01/msg01317.php
-- Euler Taveira de Oliveira http://www.timbira.com/
2010/1/24 Euler Taveira de Oliveira <euler@timbira.com>:
> Tom Lane escreveu:
>> That implies that the operations wouldn't work against system tables;
>> which they do. I think a bigger problem is that "reset_single_table"
>> seems like it might be talking about something like a TRUNCATE, ie,
>> it's not clear that it means to reset counters rather than data.
>> The pg_stat_ prefix is some help but not enough IMO. So I suggest
>> pg_stat_reset_table_counters and pg_stat_reset_function_counters.
>>
> Sure, much better. +1.
>
>> (BTW, a similar complaint could be made about the previously committed
>> patch: reset shared what?)
>>
> BTW, what about that idea to overload pg_stat_reset()? The
> pg_stat_reset_shared should be renamed to pg_stat_reset('foo') [1] where foo
> is the class of objects that it is resetting. pg_stat_reset is not a so
> suggestive name but that's one we already have; besides, it will be intuitive
> for users.
I think it's easier to use the way it is now. But yes, we could
overload it to make it:
pg_stat_reset() : everything, like now
pg_stat_reset('bgwriter') : what pg_stat_reset_shared() does
now. Can take more params.
pg_stat_reset('table', 'foo'::regclass); : what
pg_stat_reset_single_table_counters does now
The advantage would be fewer functions, but I still think it's easier
to use the way we have it now.
-- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/