Re: Stats collector performance improvement - Mailing list pgsql-patches

Bruce Momjian wrote:
> Tom Lane wrote:
> A second improvement I discovered is that the statistics collector is
> calling gettimeofday() for every packet received, so it can determine
> the timeout for the select() call to write the flat file.  I removed
> that behavior and instead used setitimer() to issue a SIGINT every
> 500ms, which was the original behavior.  This eliminates the
> gettimeofday() call and makes the code cleaner.  Second patch attached.

I have applied this second patch, with a few small stylistic
improvements.

--
  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.116
diff -c -c -r1.116 pgstat.c
*** src/backend/postmaster/pgstat.c    2 Jan 2006 00:58:00 -0000    1.116
--- src/backend/postmaster/pgstat.c    3 Jan 2006 16:26:04 -0000
***************
*** 117,123 ****

  static long pgStatNumMessages = 0;

! static bool pgStatRunningInCollector = FALSE;

  /*
   * Place where backends store per-table info to be sent to the collector.
--- 117,123 ----

  static long pgStatNumMessages = 0;

! static bool pgStatRunningInCollector = false;

  /*
   * Place where backends store per-table info to be sent to the collector.
***************
*** 145,150 ****
--- 145,151 ----
  static PgStat_StatBeEntry *pgStatBeTable = NULL;
  static int    pgStatNumBackends = 0;

+ static volatile bool    need_statwrite;

  /* ----------
   * Local function forward declarations
***************
*** 164,169 ****
--- 165,171 ----

  NON_EXEC_STATIC void PgstatBufferMain(int argc, char *argv[]);
  NON_EXEC_STATIC void PgstatCollectorMain(int argc, char *argv[]);
+ static void force_statwrite(SIGNAL_ARGS);
  static void pgstat_recvbuffer(void);
  static void pgstat_exit(SIGNAL_ARGS);
  static void pgstat_die(SIGNAL_ARGS);
***************
*** 1548,1560 ****
      PgStat_Msg    msg;
      fd_set        rfds;
      int            readPipe;
-     int            nready;
      int            len = 0;
!     struct timeval timeout;
!     struct timeval next_statwrite;
!     bool        need_statwrite;
      HASHCTL        hash_ctl;
!
      MyProcPid = getpid();        /* reset MyProcPid */

      /*
--- 1550,1560 ----
      PgStat_Msg    msg;
      fd_set        rfds;
      int            readPipe;
      int            len = 0;
!     struct itimerval timeval;
      HASHCTL        hash_ctl;
!     bool        need_timer = false;
!
      MyProcPid = getpid();        /* reset MyProcPid */

      /*
***************
*** 1572,1578 ****
      /* kluge to allow buffer process to kill collector; FIXME */
      pqsignal(SIGQUIT, pgstat_exit);
  #endif
!     pqsignal(SIGALRM, SIG_IGN);
      pqsignal(SIGPIPE, SIG_IGN);
      pqsignal(SIGUSR1, SIG_IGN);
      pqsignal(SIGUSR2, SIG_IGN);
--- 1572,1578 ----
      /* kluge to allow buffer process to kill collector; FIXME */
      pqsignal(SIGQUIT, pgstat_exit);
  #endif
!     pqsignal(SIGALRM, force_statwrite);
      pqsignal(SIGPIPE, SIG_IGN);
      pqsignal(SIGUSR1, SIG_IGN);
      pqsignal(SIGUSR2, SIG_IGN);
***************
*** 1597,1613 ****
      init_ps_display("stats collector process", "", "");
      set_ps_display("");

!     /*
!      * Arrange to write the initial status file right away
!      */
!     gettimeofday(&next_statwrite, NULL);
!     need_statwrite = TRUE;

      /*
       * Read in an existing statistics stats file or initialize the stats to
       * zero.
       */
!     pgStatRunningInCollector = TRUE;
      pgstat_read_statsfile(&pgStatDBHash, InvalidOid, NULL, NULL);

      /*
--- 1597,1613 ----
      init_ps_display("stats collector process", "", "");
      set_ps_display("");

!     need_statwrite = true;
!
!     MemSet(&timeval, 0, sizeof(struct itimerval));
!     timeval.it_value.tv_sec = PGSTAT_STAT_INTERVAL / 1000;
!     timeval.it_value.tv_usec = PGSTAT_STAT_INTERVAL % 1000;

      /*
       * Read in an existing statistics stats file or initialize the stats to
       * zero.
       */
!     pgStatRunningInCollector = true;
      pgstat_read_statsfile(&pgStatDBHash, InvalidOid, NULL, NULL);

      /*
***************
*** 1634,1667 ****
       */
      for (;;)
      {
-         /*
-          * If we need to write the status file again (there have been changes
-          * in the statistics since we wrote it last) calculate the timeout
-          * until we have to do so.
-          */
          if (need_statwrite)
          {
!             struct timeval now;
!
!             gettimeofday(&now, NULL);
!             /* avoid assuming that tv_sec is signed */
!             if (now.tv_sec > next_statwrite.tv_sec ||
!                 (now.tv_sec == next_statwrite.tv_sec &&
!                  now.tv_usec >= next_statwrite.tv_usec))
!             {
!                 timeout.tv_sec = 0;
!                 timeout.tv_usec = 0;
!             }
!             else
!             {
!                 timeout.tv_sec = next_statwrite.tv_sec - now.tv_sec;
!                 timeout.tv_usec = next_statwrite.tv_usec - now.tv_usec;
!                 if (timeout.tv_usec < 0)
!                 {
!                     timeout.tv_sec--;
!                     timeout.tv_usec += 1000000;
!                 }
!             }
          }

          /*
--- 1634,1644 ----
       */
      for (;;)
      {
          if (need_statwrite)
          {
!             pgstat_write_statsfile();
!             need_statwrite = false;
!             need_timer = true;
          }

          /*
***************
*** 1673,1681 ****
          /*
           * Now wait for something to do.
           */
!         nready = select(readPipe + 1, &rfds, NULL, NULL,
!                         (need_statwrite) ? &timeout : NULL);
!         if (nready < 0)
          {
              if (errno == EINTR)
                  continue;
--- 1650,1656 ----
          /*
           * Now wait for something to do.
           */
!         if (select(readPipe + 1, &rfds, NULL, NULL, NULL) < 0)
          {
              if (errno == EINTR)
                  continue;
***************
*** 1685,1702 ****
          }

          /*
-          * If there are no descriptors ready, our timeout for writing the
-          * stats file happened.
-          */
-         if (nready == 0)
-         {
-             pgstat_write_statsfile();
-             need_statwrite = FALSE;
-
-             continue;
-         }
-
-         /*
           * Check if there is a new statistics message to collect.
           */
          if (FD_ISSET(readPipe, &rfds))
--- 1660,1665 ----
***************
*** 1813,1829 ****
               */
              pgStatNumMessages++;

!             /*
!              * If this is the first message after we wrote the stats file the
!              * last time, setup the timeout that it'd be written.
!              */
!             if (!need_statwrite)
              {
!                 gettimeofday(&next_statwrite, NULL);
!                 next_statwrite.tv_usec += ((PGSTAT_STAT_INTERVAL) * 1000);
!                 next_statwrite.tv_sec += (next_statwrite.tv_usec / 1000000);
!                 next_statwrite.tv_usec %= 1000000;
!                 need_statwrite = TRUE;
              }
          }

--- 1776,1787 ----
               */
              pgStatNumMessages++;

!             if (need_timer)
              {
!                 if (setitimer(ITIMER_REAL, &timeval, NULL))
!                     ereport(ERROR,
!                           (errmsg("unable to set statistics collector timer: %m")));
!                 need_timer = false;
              }
          }

***************
*** 1848,1853 ****
--- 1806,1818 ----
  }


+ static void
+ force_statwrite(SIGNAL_ARGS)
+ {
+     need_statwrite = true;
+ }
+
+
  /* ----------
   * pgstat_recvbuffer() -
   *
***************
*** 1865,1871 ****
      struct timeval timeout;
      int            writePipe = pgStatPipe[1];
      int            maxfd;
-     int            nready;
      int            len;
      int            xfr;
      int            frm;
--- 1830,1835 ----
***************
*** 1907,1912 ****
--- 1871,1884 ----
      msgbuffer = (char *) palloc(PGSTAT_RECVBUFFERSZ);

      /*
+      * Wait for some work to do; but not for more than 10 seconds. (This
+      * determines how quickly we will shut down after an ungraceful
+      * postmaster termination; so it needn't be very fast.)
+      */
+     timeout.tv_sec = 10;
+     timeout.tv_usec = 0;
+
+     /*
       * Loop forever
       */
      for (;;)
***************
*** 1946,1961 ****
                  maxfd = writePipe;
          }

!         /*
!          * Wait for some work to do; but not for more than 10 seconds. (This
!          * determines how quickly we will shut down after an ungraceful
!          * postmaster termination; so it needn't be very fast.)
!          */
!         timeout.tv_sec = 10;
!         timeout.tv_usec = 0;
!
!         nready = select(maxfd + 1, &rfds, &wfds, NULL, &timeout);
!         if (nready < 0)
          {
              if (errno == EINTR)
                  continue;
--- 1918,1924 ----
                  maxfd = writePipe;
          }

!         if (select(maxfd + 1, &rfds, &wfds, NULL, &timeout) < 0)
          {
              if (errno == EINTR)
                  continue;