Re: 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(at)hagander(dot)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(at)sss(dot)pgh(dot)pa(dot)us>:
> Magnus Hagander <magnus(at)hagander(dot)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 Hagander
Me: 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(at)sss(dot)pgh(dot)pa(dot)us>:
> > Magnus Hagander <magnus(at)hagander(dot)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(at)hagander(dot)net>:
> 2010/1/24 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> Magnus Hagander <magnus(at)hagander(dot)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 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/

Magnus Hagander <magnus(at)hagander(dot)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

Euler Taveira de Oliveira <euler(at)timbira(dot)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(at)sss(dot)pgh(dot)pa(dot)us>:
> Euler Taveira de Oliveira <euler(at)timbira(dot)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 Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> 2010/1/24 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us>:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> 2010/1/24 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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 Hagander
Me: 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(at)timbira(dot)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 Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/