Thread: Checks for command string
Does anyone know why we test for pgstat_collect_querystring in routines that obviously dump only block and row-level statistics and database commit/rollback total? Is it a copy/paste error? Patch attached for review. The inclusion of pgstat_collect_querystring in these tests seems like a bug. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073 Index: src/backend/postmaster/pgstat.c =================================================================== RCS file: /cvsroot/pgsql/src/backend/postmaster/pgstat.c,v retrieving revision 1.115 diff -c -c -r1.115 pgstat.c *** src/backend/postmaster/pgstat.c 31 Dec 2005 19:39:10 -0000 1.115 --- src/backend/postmaster/pgstat.c 1 Jan 2006 03:31:24 -0000 *************** *** 810,817 **** int i; if (pgStatSock < 0 || ! !(pgstat_collect_querystring || ! pgstat_collect_tuplelevel || pgstat_collect_blocklevel)) { /* Not reporting stats, so just flush whatever we have */ --- 810,816 ---- int i; if (pgStatSock < 0 || ! !(pgstat_collect_tuplelevel || pgstat_collect_blocklevel)) { /* Not reporting stats, so just flush whatever we have */ *************** *** 1224,1231 **** void pgstat_count_xact_commit(void) { ! if (!(pgstat_collect_querystring || ! pgstat_collect_tuplelevel || pgstat_collect_blocklevel)) return; --- 1223,1229 ---- void pgstat_count_xact_commit(void) { ! if (!(pgstat_collect_tuplelevel || pgstat_collect_blocklevel)) return; *************** *** 1256,1263 **** void pgstat_count_xact_rollback(void) { ! if (!(pgstat_collect_querystring || ! pgstat_collect_tuplelevel || pgstat_collect_blocklevel)) return; --- 1254,1260 ---- void pgstat_count_xact_rollback(void) { ! if (!(pgstat_collect_tuplelevel || pgstat_collect_blocklevel)) return;
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Does anyone know why we test for pgstat_collect_querystring in routines
> that obviously dump only block and row-level statistics and database
> commit/rollback total?
Because we want commits/rollbacks to be counted if any of them are on.
regards, tom lane
Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Does anyone know why we test for pgstat_collect_querystring in routines
> > that obviously dump only block and row-level statistics and database
> > commit/rollback total?
>
> Because we want commits/rollbacks to be counted if any of them are on.
Why do we want commits/rollbacks counted if we only have command string
enabled? Here is the test:
if (pgStatSock < 0 ||
!(pgstat_collect_querystring ||
pgstat_collect_tuplelevel ||
pgstat_collect_blocklevel))
and here are the functions that have it where I think it is wrong:
pgstat_report_tabstat()
pgstat_count_xact_commit()
pgstat_count_xact_rollback()
The stats_command_string documention makes no mention of this:
Enables the collection of statistics on the currently
executing command of each session, along with the time at
which that command began execution. This option is off by
default. Note that even when enabled, this information is not
visible to all users, only to superusers and the user owning
the session being reported on; so it should not represent a
security risk. This data can be accessed via the
<structname>pg_stat_activity</structname> system view; refer
to <xref linkend="monitoring"> for more information.
Seems we should document this somewhere even if the behavior is correct,
which I don't think it is.
The !(x || y) construct is really ugly and I will fix that in a simple
commit now.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Tom Lane wrote:
>> Because we want commits/rollbacks to be counted if any of them are on.
> Why do we want commits/rollbacks counted if we only have command string
> enabled?
Why not? Those counts are not either "tuple level" or "block level"
operations; the fact that the implementation sends them in the same
messages doesn't mean that there is any association in the user's eye.
Barring making a fourth GUC variable to control them (which seems like
overkill), I think it's a reasonably sane definition to say "we count
these if any stats are being collected". Doing what you propose would
simply expose an irrelevant implementation detail to users.
> The !(x || y) construct is really ugly and I will fix that in a simple
> commit now.
I can't agree with you on that opinion, either.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Tom Lane wrote: > >> Because we want commits/rollbacks to be counted if any of them are on. > > > Why do we want commits/rollbacks counted if we only have command string > > enabled? > > Why not? Those counts are not either "tuple level" or "block level" > operations; the fact that the implementation sends them in the same > messages doesn't mean that there is any association in the user's eye. > Barring making a fourth GUC variable to control them (which seems like > overkill), I think it's a reasonably sane definition to say "we count > these if any stats are being collected". Doing what you propose would > simply expose an irrelevant implementation detail to users. OK. Don't we need to document this somewhere? > > The !(x || y) construct is really ugly and I will fix that in a simple > > commit now. > > I can't agree with you on that opinion, either. Oops, done. The good news is I found out why stat_command_string is causing such a large performance hit. I will post tonight or tomorrow on it. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Tom Lane wrote:
>> Barring making a fourth GUC variable to control them (which seems like
>> overkill), I think it's a reasonably sane definition to say "we count
>> these if any stats are being collected". Doing what you propose would
>> simply expose an irrelevant implementation detail to users.
> OK. Don't we need to document this somewhere?
No objection to that. Probably section 24.2.1 is a reasonable place?
regards, tom lane
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> The good news is I found out why stat_command_string is causing such a
> large performance hit. I will post tonight or tomorrow on it.
There's more to it than just a lot of messages being sent?
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > The good news is I found out why stat_command_string is causing such a > > large performance hit. I will post tonight or tomorrow on it. > > There's more to it than just a lot of messages being sent? You bet! I will try to post soon. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073