Thread: Re: [BUGS] BUG #1707: statistics collector starts with stats_start_collector
Re: [BUGS] BUG #1707: statistics collector starts with stats_start_collector
From
Bruce Momjian
Date:
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Later such as in a later postmaster start, but personally I would just > > remove the option completely. > > I don't have a problem with removing it as a writable option ... but > I'm thinking we should leave it as a read-only GUC parameter (like > the several others we have already). Otherwise we'll need to add some > other method of finding out whether the collector is running. I assume the collector should always be running, and if it isn't, it is mentioned in the server logs. That seems sufficient. The following patch removes the GUC stats_start_collector. The stats process is now always started. -- 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: doc/src/sgml/monitoring.sgml =================================================================== RCS file: /cvsroot/pgsql/doc/src/sgml/monitoring.sgml,v retrieving revision 1.28 diff -c -c -r1.28 monitoring.sgml *** doc/src/sgml/monitoring.sgml 9 May 2005 11:31:32 -0000 1.28 --- doc/src/sgml/monitoring.sgml 28 Jun 2005 00:38:07 -0000 *************** *** 128,143 **** </para> <para> - The parameter <xref linkend="guc-stats-start-collector"> must be - set to <literal>true</> for the statistics collector to be launched - at all. This is the default and recommended setting, but it may be - turned off if you have no interest in statistics and want to - squeeze out every last drop of overhead. (The savings is likely to - be small, however.) Note that this option cannot be changed while - the server is running. - </para> - - <para> The parameters <xref linkend="guc-stats-command-string">, <xref linkend="guc-stats-block-level">, and <xref linkend="guc-stats-row-level"> control how much information is --- 128,133 ---- Index: doc/src/sgml/runtime.sgml =================================================================== RCS file: /cvsroot/pgsql/doc/src/sgml/runtime.sgml,v retrieving revision 1.332 diff -c -c -r1.332 runtime.sgml *** doc/src/sgml/runtime.sgml 26 Jun 2005 19:16:04 -0000 1.332 --- doc/src/sgml/runtime.sgml 28 Jun 2005 00:38:11 -0000 *************** *** 3057,3078 **** <title>Query and Index Statistics Collector</title> <variablelist> - <varlistentry id="guc-stats-start-collector" xreflabel="stats_start_collector"> - <term><varname>stats_start_collector</varname> (<type>boolean</type>)</term> - <indexterm> - <primary><varname>stats_start_collector</> configuration parameter</primary> - </indexterm> - <listitem> - <para> - Controls whether the server should start the - statistics-collection subprocess. This is on by default, but - may be turned off if you know you have no interest in - collecting statistics. This option can only be set at server - start. - </para> - </listitem> - </varlistentry> - <varlistentry id="guc-stats-command-string" xreflabel="stats_command_string"> <term><varname>stats_command_string</varname> (<type>boolean</type>)</term> <indexterm> --- 3057,3062 ---- Index: src/backend/postmaster/pgstat.c =================================================================== RCS file: /cvsroot/pgsql/src/backend/postmaster/pgstat.c,v retrieving revision 1.96 diff -c -c -r1.96 pgstat.c *** src/backend/postmaster/pgstat.c 25 Jun 2005 23:58:57 -0000 1.96 --- src/backend/postmaster/pgstat.c 28 Jun 2005 00:38:13 -0000 *************** *** 99,105 **** * GUC parameters * ---------- */ - bool pgstat_collect_startcollector = true; bool pgstat_collect_resetonpmstart = true; bool pgstat_collect_querystring = false; bool pgstat_collect_tuplelevel = false; --- 99,104 ---- *************** *** 214,227 **** #define TESTBYTEVAL ((char) 199) /* - * Force start of collector daemon if something to collect - */ - if (pgstat_collect_querystring || - pgstat_collect_tuplelevel || - pgstat_collect_blocklevel) - pgstat_collect_startcollector = true; - - /* * Initialize the filename for the status reports. (In the * EXEC_BACKEND case, this only sets the value in the postmaster. The * collector subprocess will recompute the value for itself, and --- 213,218 ---- *************** *** 231,249 **** snprintf(pgStat_fname, MAXPGPATH, PGSTAT_STAT_FILENAME, DataDir); /* ! * If we don't have to start a collector or should reset the collected ! * statistics on postmaster start, simply remove the file. */ ! if (!pgstat_collect_startcollector || pgstat_collect_resetonpmstart) unlink(pgStat_fname); /* - * Nothing else required if collector will not get started - */ - if (!pgstat_collect_startcollector) - return; - - /* * Create the UDP socket for sending and receiving statistic messages */ hints.ai_flags = AI_PASSIVE; --- 222,234 ---- snprintf(pgStat_fname, MAXPGPATH, PGSTAT_STAT_FILENAME, DataDir); /* ! * If we should reset the collected statistics on postmaster start, ! * simply remove the file. */ ! if (pgstat_collect_resetonpmstart) unlink(pgStat_fname); /* * Create the UDP socket for sending and receiving statistic messages */ hints.ai_flags = AI_PASSIVE; *************** *** 448,454 **** pgStatSock = -1; /* Adjust GUC variables to suppress useless activity */ - pgstat_collect_startcollector = false; pgstat_collect_querystring = false; pgstat_collect_tuplelevel = false; pgstat_collect_blocklevel = false; --- 433,438 ---- *************** *** 540,551 **** pid_t pgStatPid; /* - * Do nothing if no collector needed - */ - if (!pgstat_collect_startcollector) - return 0; - - /* * Do nothing if too soon since last collector start. This is a * safety valve to protect against continuous respawn attempts if the * collector is dying immediately at launch. Note that since we will --- 524,529 ---- *************** *** 559,580 **** last_pgstat_start_time = curtime; /* - * Check that the socket is there, else pgstat_init failed. - */ - if (pgStatSock < 0) - { - ereport(LOG, - (errmsg("statistics collector startup skipped"))); - - /* - * We can only get here if someone tries to manually turn - * pgstat_collect_startcollector on after it had been off. - */ - pgstat_collect_startcollector = false; - return 0; - } - - /* * Okay, fork off the collector. */ #ifdef EXEC_BACKEND --- 537,542 ---- Index: src/backend/utils/misc/guc.c =================================================================== RCS file: /cvsroot/pgsql/src/backend/utils/misc/guc.c,v retrieving revision 1.270 diff -c -c -r1.270 guc.c *** src/backend/utils/misc/guc.c 26 Jun 2005 19:16:06 -0000 1.270 --- src/backend/utils/misc/guc.c 28 Jun 2005 00:38:16 -0000 *************** *** 622,635 **** true, NULL, NULL }, { - {"stats_start_collector", PGC_POSTMASTER, STATS_COLLECTOR, - gettext_noop("Starts the server statistics-collection subprocess."), - NULL - }, - &pgstat_collect_startcollector, - true, NULL, NULL - }, - { {"stats_reset_on_server_start", PGC_POSTMASTER, STATS_COLLECTOR, gettext_noop("Zeroes collected statistics on server restart."), NULL --- 622,627 ---- Index: src/backend/utils/misc/postgresql.conf.sample =================================================================== RCS file: /cvsroot/pgsql/src/backend/utils/misc/postgresql.conf.sample,v retrieving revision 1.148 diff -c -c -r1.148 postgresql.conf.sample *** src/backend/utils/misc/postgresql.conf.sample 26 Jun 2005 03:03:41 -0000 1.148 --- src/backend/utils/misc/postgresql.conf.sample 28 Jun 2005 00:38:16 -0000 *************** *** 272,278 **** # - Query/Index Statistics Collector - - #stats_start_collector = true #stats_command_string = false #stats_block_level = false #stats_row_level = false --- 272,277 ---- Index: src/bin/psql/tab-complete.c =================================================================== RCS file: /cvsroot/pgsql/src/bin/psql/tab-complete.c,v retrieving revision 1.133 diff -c -c -r1.133 tab-complete.c *** src/bin/psql/tab-complete.c 22 Jun 2005 21:14:30 -0000 1.133 --- src/bin/psql/tab-complete.c 28 Jun 2005 00:38:18 -0000 *************** *** 596,602 **** "stats_command_string", "stats_reset_on_server_start", "stats_row_level", - "stats_start_collector", "superuser_reserved_connections", "syslog_facility", "syslog_ident", --- 596,601 ---- Index: src/include/pgstat.h =================================================================== RCS file: /cvsroot/pgsql/src/include/pgstat.h,v retrieving revision 1.30 diff -c -c -r1.30 pgstat.h *** src/include/pgstat.h 25 Jun 2005 23:58:58 -0000 1.30 --- src/include/pgstat.h 28 Jun 2005 00:38:19 -0000 *************** *** 295,301 **** * GUC parameters * ---------- */ - extern bool pgstat_collect_startcollector; extern bool pgstat_collect_resetonpmstart; extern bool pgstat_collect_querystring; extern bool pgstat_collect_tuplelevel; --- 295,300 ---- Index: src/test/regress/expected/stats.out =================================================================== RCS file: /cvsroot/pgsql/src/test/regress/expected/stats.out,v retrieving revision 1.4 diff -c -c -r1.4 stats.out *** src/test/regress/expected/stats.out 22 Apr 2005 21:58:32 -0000 1.4 --- src/test/regress/expected/stats.out 28 Jun 2005 00:38:20 -0000 *************** *** 4,16 **** -- Must be run after tenk2 has been created (by create_table), -- populated (by create_misc) and indexed (by create_index). -- - -- conditio sine qua non - SHOW stats_start_collector; -- must be on - stats_start_collector - ----------------------- - on - (1 row) - -- XXX stopgap until we figure out how bitmap scans should be counted SET enable_bitmapscan = off; -- save counters --- 4,9 ---- Index: src/test/regress/sql/stats.sql =================================================================== RCS file: /cvsroot/pgsql/src/test/regress/sql/stats.sql,v retrieving revision 1.2 diff -c -c -r1.2 stats.sql *** src/test/regress/sql/stats.sql 22 Apr 2005 21:58:32 -0000 1.2 --- src/test/regress/sql/stats.sql 28 Jun 2005 00:38:20 -0000 *************** *** 5,13 **** -- populated (by create_misc) and indexed (by create_index). -- - -- conditio sine qua non - SHOW stats_start_collector; -- must be on - -- XXX stopgap until we figure out how bitmap scans should be counted SET enable_bitmapscan = off; --- 5,10 ----
Re: [BUGS] BUG #1707: statistics collector starts with stats_start_collector = false
From
Tom Lane
Date:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> The following patch removes the GUC stats_start_collector. The stats
> process is now always started.
WAIT a second!!!
That was *not* the agenda; the collector is only supposed to start if
you turn on at least one of the collection options.
regards, tom lane
Re: [BUGS] BUG #1707: statistics collector starts with stats_start_collector
From
Bruce Momjian
Date:
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > The following patch removes the GUC stats_start_collector. The stats > > process is now always started. > > WAIT a second!!! > > That was *not* the agenda; the collector is only supposed to start if > you turn on at least one of the collection options. Really? I thought the process always started and it just sits there if nothing is fed to it, no? In fact, if it wasn't started from the beginning, we couldn't enable query strings without restarting the server. The current default is true, meaning it always started. -- 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
Re: [BUGS] BUG #1707: statistics collector starts with stats_start_collector = false
From
Tom Lane
Date:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> The current default is true, meaning it always started.
*By default* it starts. But you can turn it off, and there was no
discussion of removing that option.
regards, tom lane
Re: [BUGS] BUG #1707: statistics collector starts with stats_start_collector
From
Bruce Momjian
Date:
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > The current default is true, meaning it always started. > > *By default* it starts. But you can turn it off, and there was no > discussion of removing that option. I thought we agreed that there was little value in allowing someone to prevent the collector from starting. It is just a process slot, right? You were suggesting we make it read-only in the email I quoted originally. I am confused. -- 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
Re: [BUGS] BUG #1707: statistics collector starts with stats_start_collector
From
Alvaro Herrera
Date:
On Mon, Jun 27, 2005 at 08:51:46PM -0400, Bruce Momjian wrote: > Tom Lane wrote: > > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > > The following patch removes the GUC stats_start_collector. The stats > > > process is now always started. > > > > WAIT a second!!! > > > > That was *not* the agenda; the collector is only supposed to start if > > you turn on at least one of the collection options. > > Really? I thought the process always started and it just sits there if > nothing is fed to it, no? No, the process is only started if you ask for it (which is true by default). > In fact, if it wasn't started from the beginning, we couldn't enable > query strings without restarting the server. That's true -- if you start with the collector disabled, you can't enable query strings. > The current default is true, meaning it always started. Yes, but you can turn it off. -- Alvaro Herrera (<alvherre[a]surnet.cl>) "La libertad es como el dinero; el que no la sabe emplear la pierde" (Alvarez)
Re: [BUGS] BUG #1707: statistics collector starts with stats_start_collector
From
Bruce Momjian
Date:
Alvaro Herrera wrote: > On Mon, Jun 27, 2005 at 08:51:46PM -0400, Bruce Momjian wrote: > > Tom Lane wrote: > > > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > > > The following patch removes the GUC stats_start_collector. The stats > > > > process is now always started. > > > > > > WAIT a second!!! > > > > > > That was *not* the agenda; the collector is only supposed to start if > > > you turn on at least one of the collection options. > > > > Really? I thought the process always started and it just sits there if > > nothing is fed to it, no? > > No, the process is only started if you ask for it (which is true by > default). > > > In fact, if it wasn't started from the beginning, we couldn't enable > > query strings without restarting the server. > > That's true -- if you start with the collector disabled, you can't > enable query strings. > > > The current default is true, meaning it always started. > > Yes, but you can turn it off. OK, but if all the stats* variables are off, what overhead is there in having the stats process run? I would think very little, and if we turn it always on, we make configuration easier and remove a GUC variable. -- 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
Re: [BUGS] BUG #1707: statistics collector starts with stats_start_collector
From
Alvaro Herrera
Date:
On Mon, Jun 27, 2005 at 09:13:20PM -0400, Bruce Momjian wrote: > Alvaro Herrera wrote: > > > In fact, if it wasn't started from the beginning, we couldn't enable > > > query strings without restarting the server. > > > > That's true -- if you start with the collector disabled, you can't > > enable query strings. > > OK, but if all the stats* variables are off, what overhead is there in > having the stats process run? I would think very little, and if we turn > it always on, we make configuration easier and remove a GUC variable. What about arranging postmaster's main loop so that it starts the stats collector process if one of the options is activated (assuming they were all off at start)? Right now you have to restart the server, but I don't see why it has to be like that. That has even less overhead. We will have to keep the GUC var, but I don't think that's too onerous, is it? -- Alvaro Herrera (<alvherre[a]surnet.cl>) "Granting software the freedom to evolve guarantees only different results, not better ones." (Zygo Blaxell)
Alvaro Herrera <alvherre@surnet.cl> writes:
> What about arranging postmaster's main loop so that it starts the stats
> collector process if one of the options is activated (assuming they were
> all off at start)? Right now you have to restart the server, but I
> don't see why it has to be like that.
Actually, this entire thread is based on a misconception: the "bug"
reporter was under the misimpression that the sum total of what the
collector does is described by the three suboptions, and so having
start_collector on without any of the suboptions on is a useless state.
But this is not so. The collector tracks backend start/end in any case,
and so you get a useful list of sessions in pg_stat_activity even
without enabling any of the more expensive reporting options.
So my vote at this point is that the code is not broken and we should
not change it. Possibly the documentation could stand a little
improvement.
regards, tom lane
Alvaro Herrera <alvherre@surnet.cl> writes:
> What about arranging postmaster's main loop so that it starts the stats
> collector process if one of the options is activated (assuming they were
> all off at start)? Right now you have to restart the server, but I
> don't see why it has to be like that.
The reason for a restart is that what "stats_start_collector" is REALLY
all about is telling the postmaster whether to establish the loopback
UDP socket for stats messaging. You don't get to add that after the
fact because there'd be no way to propagate the open socket into
existing backends.
And no, I wouldn't look with favor on the idea of creating the socket
unconditionally. One of the reasons for wanting to be able to turn
off pgstats entirely is so that you aren't dead in the water if the
loopback UDP socket won't work for some reason (example: kernel packet
filtering that's not under your control).
regards, tom lane
Re: [BUGS] BUG #1707: statistics collector starts with stats_start_collector
From
Bruce Momjian
Date:
Tom Lane wrote: > Actually, this entire thread is based on a misconception: the "bug" > reporter was under the misimpression that the sum total of what the > collector does is described by the three suboptions, and so having > start_collector on without any of the suboptions on is a useless state. > But this is not so. The collector tracks backend start/end in any case, > and so you get a useful list of sessions in pg_stat_activity even > without enabling any of the more expensive reporting options. Right. > So my vote at this point is that the code is not broken and we should > not change it. Possibly the documentation could stand a little > improvement. No question. > Alvaro Herrera <alvherre@surnet.cl> writes: > > What about arranging postmaster's main loop so that it starts the stats > > collector process if one of the options is activated (assuming they were > > all off at start)? Right now you have to restart the server, but I > > don't see why it has to be like that. > > The reason for a restart is that what "stats_start_collector" is REALLY > all about is telling the postmaster whether to establish the loopback > UDP socket for stats messaging. You don't get to add that after the > fact because there'd be no way to propagate the open socket into > existing backends. > > And no, I wouldn't look with favor on the idea of creating the socket > unconditionally. One of the reasons for wanting to be able to turn > off pgstats entirely is so that you aren't dead in the water if the > loopback UDP socket won't work for some reason (example: kernel packet > filtering that's not under your control). My issue is that we usually don't have GUC variables that can be set automatically. Why can't we have the stats collector try to start, and just throw a server log message if it fails. I guess I don't see why that is worse than a GUC. Why is the stats collector unique in its use of of the UDP resource? If we can't start IPv6 we just throw a server log message and contiue, right? -- 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:
> My issue is that we usually don't have GUC variables that can be set
> automatically. Why can't we have the stats collector try to start, and
> just throw a server log message if it fails.
I still say the code is not broken and does not require fixing. If
someone doesn't want any stats collection service, they shouldn't have
to pay the overhead of having the collector there. That was the
original design intention for pgstats, and there isn't anyone except
you suggesting that we should force people to have the stats collector
enabled.
regards, tom lane