Re: temporary statistics option at initdb time - Mailing list pgsql-hackers

Magnus Hagander wrote:
> Decibel! wrote:
>> On Aug 13, 2008, at 4:12 AM, Magnus Hagander wrote:
>>> Tom Lane wrote:
>>>> Decibel! <decibel@decibel.org> writes:
>>>>> I disagree. While we don't guarantee stats are absolutely up-to-date,
>>>>> or atomic I don't think that gives license for them to just magically
>>>>> not exist sometimes.
>>>>> Would it really be that hard to have the system copy the file out
>>>>> before telling all the other backends of the change?
>>>> Well, there is no (zero, zilch, nada) use-case for changing this setting
>>>> on the fly.  Why not make it a "frozen at postmaster start" GUC?  Seems
>>>> like that gets all the functionality needed and most of the ease of use.
>>> Oh, there is a use-case. If you run your system and then only afterwards
>>> realize the I/O from the stats file is high enough to be an issue, and
>>> want to change it.
>>>
>>> That said, I'm not sure the use-case is anywhere near common enough to
>>> put a lot of code into it.
>>
>> Something to keep in mind as PG is used to build larger systems 'further
>> up the enterprise'... for us to bounce a database at work costs us a LOT
>> in lost revenue. I don't want to go into specifics, but it's more than
>> enough to buy a very nice car. :) That's why I asked how hard it'd be to
>> do this on the fly.
>
> Well, it's doable, but fairly hard.
>
> But you can do it the symlink way without shutting it down, I think. :-)

Actually, I think maybe not so hard. Attached is a patch that fixes
this. It's done by keeping the old filename around. When you change the
path, the stats collector will start writing the new file the next time
it writes something (which should be max 0.5 seconds later if something
is happening). The backends will immediately try to read from the new
filename, but if that one is not found, they will switch to reading the
old filename. This obviously fails if you change the temp directory
twice in less than half a second, but I really don't see a use-case for
that...

Or did I miss something here? :-)

//Magnus
Index: backend/postmaster/pgstat.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/postmaster/pgstat.c,v
retrieving revision 1.179
diff -c -r1.179 pgstat.c
*** backend/postmaster/pgstat.c    15 Aug 2008 08:37:39 -0000    1.179
--- backend/postmaster/pgstat.c    19 Aug 2008 15:24:59 -0000
***************
*** 110,115 ****
--- 110,116 ----
   */
  char       *pgstat_stat_filename = NULL;
  char       *pgstat_stat_tmpname = NULL;
+ char       *pgstat_stat_lastname = NULL;

  /*
   * BgWriter global statistics counters (unused in other processes).
***************
*** 203,208 ****
--- 204,210 ----

  static volatile bool need_exit = false;
  static volatile bool need_statwrite = false;
+ static volatile bool got_SIGHUP = false;

  /*
   * Total time charged to functions so far in the current backend.
***************
*** 224,229 ****
--- 226,232 ----
  static void pgstat_exit(SIGNAL_ARGS);
  static void force_statwrite(SIGNAL_ARGS);
  static void pgstat_beshutdown_hook(int code, Datum arg);
+ static void pgstat_sighup_handler(SIGNAL_ARGS);

  static PgStat_StatDBEntry *pgstat_get_db_entry(Oid databaseid, bool create);
  static void pgstat_write_statsfile(bool permanent);
***************
*** 2571,2577 ****
       * Ignore all signals usually bound to some action in the postmaster,
       * except SIGQUIT and SIGALRM.
       */
!     pqsignal(SIGHUP, SIG_IGN);
      pqsignal(SIGINT, SIG_IGN);
      pqsignal(SIGTERM, SIG_IGN);
      pqsignal(SIGQUIT, pgstat_exit);
--- 2574,2580 ----
       * Ignore all signals usually bound to some action in the postmaster,
       * except SIGQUIT and SIGALRM.
       */
!     pqsignal(SIGHUP, pgstat_sighup_handler);
      pqsignal(SIGINT, SIG_IGN);
      pqsignal(SIGTERM, SIG_IGN);
      pqsignal(SIGQUIT, pgstat_exit);
***************
*** 2635,2640 ****
--- 2638,2652 ----
              break;

          /*
+          * Reload configuration if we got SIGHUP from the postmaster.
+          */
+         if (got_SIGHUP)
+         {
+             ProcessConfigFile(PGC_SIGHUP);
+             got_SIGHUP = false;
+         }
+
+         /*
           * If time to write the stats file, do so.    Note that the alarm
           * interrupt isn't re-enabled immediately, but only after we next
           * receive a stats message; so no cycles are wasted when there is
***************
*** 2834,2839 ****
--- 2846,2858 ----
      need_statwrite = true;
  }

+ /* SIGHUP handler for collector process */
+ static void
+ pgstat_sighup_handler(SIGNAL_ARGS)
+ {
+     got_SIGHUP = true;
+ }
+

  /*
   * Lookup the hash table entry for the specified database. If no hash
***************
*** 3018,3023 ****
--- 3037,3059 ----

      if (permanent)
          unlink(pgstat_stat_filename);
+     else
+     {
+         /*
+          * If the old filename exists, we need to unlink it now that we have written
+          * the new file. This indicates that we are done, and it also makes it possible
+          * to switch the directory back without getting the old data.
+          *
+          * Also do away with the old name here, so we don't try to delete it again.
+          */
+         if (pgstat_stat_lastname != NULL)
+         {
+             unlink(pgstat_stat_lastname);
+
+             free(pgstat_stat_lastname);
+             pgstat_stat_lastname = NULL;
+         }
+     }
  }


***************
*** 3072,3080 ****
       * Try to open the status file. If it doesn't exist, the backends simply
       * return zero for anything and the collector simply starts from scratch
       * with empty counters.
       */
      if ((fpin = AllocateFile(statfile, PG_BINARY_R)) == NULL)
!         return dbhash;

      /*
       * Verify it's of the expected format.
--- 3108,3128 ----
       * Try to open the status file. If it doesn't exist, the backends simply
       * return zero for anything and the collector simply starts from scratch
       * with empty counters.
+      * If we are loading the temporary file, look at pgstat_stat_lastname as
+      * well, in case the filename changed recently and the collector hasn't
+      * written the new file yet.
       */
      if ((fpin = AllocateFile(statfile, PG_BINARY_R)) == NULL)
!     {
!         if (!permanent && pgstat_stat_lastname != NULL)
!         {
!             /* attempt to load the old file if it's there */
!             if ((fpin = AllocateFile(pgstat_stat_lastname, PG_BINARY_R)) == NULL)
!                 return dbhash;
!         }
!         else
!             return dbhash;
!     }

      /*
       * Verify it's of the expected format.
Index: backend/postmaster/postmaster.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/postmaster/postmaster.c,v
retrieving revision 1.561
diff -c -r1.561 postmaster.c
*** backend/postmaster/postmaster.c    26 Jun 2008 02:47:19 -0000    1.561
--- backend/postmaster/postmaster.c    19 Aug 2008 15:24:59 -0000
***************
*** 1923,1932 ****
              signal_child(PgArchPID, SIGHUP);
          if (SysLoggerPID != 0)
              signal_child(SysLoggerPID, SIGHUP);
!         /* PgStatPID does not currently need SIGHUP */

          /* Reload authentication config files too */
          load_ident();

  #ifdef EXEC_BACKEND
--- 1931,1944 ----
              signal_child(PgArchPID, SIGHUP);
          if (SysLoggerPID != 0)
              signal_child(SysLoggerPID, SIGHUP);
!         if (PgStatPID != 0)
!             signal_child(PgStatPID, SIGHUP);

          /* Reload authentication config files too */
          load_ident();

  #ifdef EXEC_BACKEND
Index: backend/utils/misc/guc.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/misc/guc.c,v
retrieving revision 1.466
diff -c -r1.466 guc.c
*** backend/utils/misc/guc.c    15 Aug 2008 08:37:40 -0000    1.466
--- backend/utils/misc/guc.c    19 Aug 2008 15:24:59 -0000
***************
*** 2470,2476 ****
      },

      {
!         {"stats_temp_directory", PGC_POSTMASTER, STATS_COLLECTOR,
              gettext_noop("Writes temporary statistics files to the specified directory."),
              NULL,
              GUC_SUPERUSER_ONLY
--- 2470,2476 ----
      },

      {
!         {"stats_temp_directory", PGC_SIGHUP, STATS_COLLECTOR,
              gettext_noop("Writes temporary statistics files to the specified directory."),
              NULL,
              GUC_SUPERUSER_ONLY
***************
*** 7390,7397 ****
      {
          if (pgstat_stat_tmpname)
              free(pgstat_stat_tmpname);
          if (pgstat_stat_filename)
!             free(pgstat_stat_filename);

          pgstat_stat_tmpname = guc_malloc(FATAL, strlen(newval) + 12);  /* /pgstat.tmp */
          pgstat_stat_filename = guc_malloc(FATAL, strlen(newval) + 13); /* /pgstat.stat */
--- 7390,7404 ----
      {
          if (pgstat_stat_tmpname)
              free(pgstat_stat_tmpname);
+
+         /* save away the last filename used */
          if (pgstat_stat_filename)
!         {
!             if (pgstat_stat_lastname)
!                 free(pgstat_stat_lastname);
!
!             pgstat_stat_lastname = pgstat_stat_filename;
!         }

          pgstat_stat_tmpname = guc_malloc(FATAL, strlen(newval) + 12);  /* /pgstat.tmp */
          pgstat_stat_filename = guc_malloc(FATAL, strlen(newval) + 13); /* /pgstat.stat */
Index: include/pgstat.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/pgstat.h,v
retrieving revision 1.78
diff -c -r1.78 pgstat.h
*** include/pgstat.h    15 Aug 2008 08:37:40 -0000    1.78
--- include/pgstat.h    19 Aug 2008 15:24:59 -0000
***************
*** 578,583 ****
--- 578,584 ----
  extern int    pgstat_track_activity_query_size;
  extern char *pgstat_stat_tmpname;
  extern char *pgstat_stat_filename;
+ extern char *pgstat_stat_lastname;

  /*
   * BgWriter statistics counters are updated directly by bgwriter and bufmgr