Re: [BUGS] BUG #1707: statistics collector starts with stats_start_collector

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