Resetting a single statistics counter

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 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@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 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@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

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 Hagander
Me: 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 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]http://archives.postgresql.org/pgsql-hackers/2010-01/msg01317.php 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 Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/