Using stats_command_string for xact statistics

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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

I know we said we don't want to add an additional GUC variable just to
control xact statistics, but I am thinking that using
stat_command_string isn't a logical variable to use because it is
unrelated to commutative statistics.

I am thinking using row and block-level statistics to turn on xact
statistics makes sense, but not to use stat_command_string for that
purpose.

---------------------------------------------------------------------------

Bruce Momjian wrote:
> 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(at)candle(dot)pha(dot)pa(dot)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;
>

>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/docs/faq

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)us> writes:
> I know we said we don't want to add an additional GUC variable just to
> control xact statistics, but I am thinking that using
> stat_command_string isn't a logical variable to use because it is
> unrelated to commutative statistics.

> I am thinking using row and block-level statistics to turn on xact
> statistics makes sense, but not to use stat_command_string for that
> purpose.

I don't see any strong logic to that, and changing the behavior just
for the sake of change doesn't appeal to me...

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > I know we said we don't want to add an additional GUC variable just to
> > control xact statistics, but I am thinking that using
> > stat_command_string isn't a logical variable to use because it is
> > unrelated to commutative statistics.
>
> > I am thinking using row and block-level statistics to turn on xact
> > statistics makes sense, but not to use stat_command_string for that
> > purpose.
>
> I don't see any strong logic to that, and changing the behavior just
> for the sake of change doesn't appeal to me...

OK, additional sentence added, and paragraph split into two:

! Additionally, per-database transaction commit and abort statistics
! are collected if any of these parameters are set.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073