Re: reducing statistics write overhead - Mailing list pgsql-hackers

I wrote:
> I was thinking that the launcher should only request fresh stats at wakeup,
> the workers could then reuse that file. This could be implemented by calling
> pgstat_clear_snapshot only at launcher wakeup and setting max stats age to
> to autovacuum_naptime for the workers.
>

Attached is a patch that increases the autovacuum stats age tolerance to
autovacuum_naptime. This is handled by autovac_refresh_stats() by not clearing
the stats snapshot unless nap time elapsed or explicitly forced by an error
or SIGHUP.

For the time being, I left the table vacuum recheck in place. Removing the
table_recheck_autovac function requires some further work. I have started on
this, but decided to defer until it is clear whether the whole approach is
acceptable or not.

regards,
Martin

*** a/src/backend/postmaster/autovacuum.c
--- b/src/backend/postmaster/autovacuum.c
***************
*** 44,54 ****
   * Note that there can be more than one worker in a database concurrently.
   * They will store the table they are currently vacuuming in shared memory, so
   * that other workers avoid being blocked waiting for the vacuum lock for that
!  * table.  They will also reload the pgstats data just before vacuuming each
!  * table, to avoid vacuuming a table that was just finished being vacuumed by
!  * another worker and thus is no longer noted in shared memory.  However,
!  * there is a window (caused by pgstat delay) on which a worker may choose a
!  * table that was already vacuumed; this is a bug in the current design.
   *
   * Portions Copyright (c) 1996-2009, PostgreSQL Global Development Group
   * Portions Copyright (c) 1994, Regents of the University of California
--- 44,53 ----
   * Note that there can be more than one worker in a database concurrently.
   * They will store the table they are currently vacuuming in shared memory, so
   * that other workers avoid being blocked waiting for the vacuum lock for that
!  * table. There is a possibility that a worker might pick up a table that was
!  * already vacuumed by another process. This isn't really a problem, as the
!  * odds of this happening are low and the revacuum is made cheap by the use of
!  * visibility map.
   *
   * Portions Copyright (c) 1996-2009, PostgreSQL Global Development Group
   * Portions Copyright (c) 1994, Regents of the University of California
***************
*** 120,128 **** int            autovacuum_vac_cost_limit;

  int            Log_autovacuum_min_duration = -1;

- /* how long to keep pgstat data in the launcher, in milliseconds */
- #define STATS_READ_DELAY 1000
-

  /* Flags to tell if we are in an autovacuum process */
  static bool am_autovacuum_launcher = false;
--- 119,124 ----
***************
*** 298,304 **** static void avl_sighup_handler(SIGNAL_ARGS);
  static void avl_sigusr1_handler(SIGNAL_ARGS);
  static void avl_sigterm_handler(SIGNAL_ARGS);
  static void avl_quickdie(SIGNAL_ARGS);
! static void autovac_refresh_stats(void);



--- 294,300 ----
  static void avl_sigusr1_handler(SIGNAL_ARGS);
  static void avl_sigterm_handler(SIGNAL_ARGS);
  static void avl_quickdie(SIGNAL_ARGS);
! static void autovac_refresh_stats(bool force);



***************
*** 500,509 **** AutoVacLauncherMain(int argc, char *argv[])
          DatabaseList = NULL;

          /*
!          * Make sure pgstat also considers our stat data as gone.  Note: we
!          * mustn't use autovac_refresh_stats here.
           */
!         pgstat_clear_snapshot();

          /* Now we can allow interrupts again */
          RESUME_INTERRUPTS();
--- 496,504 ----
          DatabaseList = NULL;

          /*
!          * Make sure pgstat also considers our stat data as gone.
           */
!         autovac_refresh_stats(true);

          /* Now we can allow interrupts again */
          RESUME_INTERRUPTS();
***************
*** 598,603 **** AutoVacLauncherMain(int argc, char *argv[])
--- 593,601 ----
          if (got_SIGTERM)
              break;

+         /* Refresh stats. Force it, if reloaded via SIGHUP */
+         autovac_refresh_stats(got_SIGHUP);
+
          if (got_SIGHUP)
          {
              got_SIGHUP = false;
***************
*** 851,859 **** rebuild_database_list(Oid newdb)
      int            nelems;
      HTAB       *dbhash;

-     /* use fresh stats */
-     autovac_refresh_stats();
-
      newcxt = AllocSetContextCreate(AutovacMemCxt,
                                     "AV dblist",
                                     ALLOCSET_DEFAULT_MINSIZE,
--- 849,854 ----
***************
*** 1078,1086 **** do_start_worker(void)
                                     ALLOCSET_DEFAULT_MAXSIZE);
      oldcxt = MemoryContextSwitchTo(tmpcxt);

-     /* use fresh stats */
-     autovac_refresh_stats();
-
      /* Get a list of databases */
      dblist = get_database_list();

--- 1073,1078 ----
***************
*** 2145,2158 **** do_autovacuum(void)
          }

          /*
!          * Check whether pgstat data still says we need to vacuum this table.
!          * It could have changed if something else processed the table while
!          * we weren't looking.
!          *
!          * Note: we have a special case in pgstat code to ensure that the stats
!          * we read are as up-to-date as possible, to avoid the problem that
!          * somebody just finished vacuuming this table.  The window to the race
!          * condition is not closed but it is very small.
           */
          MemoryContextSwitchTo(AutovacMemCxt);
          tab = table_recheck_autovac(relid, table_toast_map);
--- 2137,2145 ----
          }

          /*
!          * Check whether we still need to vacuum this table.  It could have
!          * changed if something else processed the table while we weren't
!          * looking.
           */
          MemoryContextSwitchTo(AutovacMemCxt);
          tab = table_recheck_autovac(relid, table_toast_map);
***************
*** 2377,2385 **** table_recheck_autovac(Oid relid, HTAB *table_toast_map)
      PgStat_StatDBEntry *dbentry;
      bool        wraparound;

-     /* use fresh stats */
-     autovac_refresh_stats();
-
      shared = pgstat_fetch_stat_dbentry(InvalidOid);
      dbentry = pgstat_fetch_stat_dbentry(MyDatabaseId);

--- 2364,2369 ----
***************
*** 2816,2844 **** AutoVacuumShmemInit(void)
   *        Refresh pgstats data for an autovacuum process
   *
   * Cause the next pgstats read operation to obtain fresh data, but throttle
!  * such refreshing in the autovacuum launcher.    This is mostly to avoid
!  * rereading the pgstats files too many times in quick succession when there
!  * are many databases.
!  *
!  * Note: we avoid throttling in the autovac worker, as it would be
!  * counterproductive in the recheck logic.
   */
  static void
! autovac_refresh_stats(void)
  {
!     if (IsAutoVacuumLauncherProcess())
!     {
!         static TimestampTz last_read = 0;
!         TimestampTz current_time;

!         current_time = GetCurrentTimestamp();

!         if (!TimestampDifferenceExceeds(last_read, current_time,
!                                         STATS_READ_DELAY))
!             return;

!         last_read = current_time;
!     }

      pgstat_clear_snapshot();
  }
--- 2800,2822 ----
   *        Refresh pgstats data for an autovacuum process
   *
   * Cause the next pgstats read operation to obtain fresh data, but throttle
!  * the refreshing so it only occurs at autovacuum_naptime intervals. This is
!  * mostly to avoid rereading the pgstats files too many times in quick
!  * succession when there are many databases.
   */
  static void
! autovac_refresh_stats(bool force)
  {
!     static TimestampTz last_read = 0;
!     TimestampTz current_time;

!     current_time = GetCurrentTimestamp();

!     if (!force && !TimestampDifferenceExceeds(last_read, current_time,
!                                               autovacuum_naptime * 1000))
!         return;

!     last_read = current_time;

      pgstat_clear_snapshot();
  }
*** a/src/backend/postmaster/pgstat.c
--- b/src/backend/postmaster/pgstat.c
***************
*** 3388,3396 **** backend_read_statsfile(void)
       * We set the minimum acceptable timestamp to PGSTAT_STAT_INTERVAL msec
       * before now.  This indirectly ensures that the collector needn't write
       * the file more often than PGSTAT_STAT_INTERVAL.  In an autovacuum
!      * worker, however, we want a lower delay to avoid using stale data, so we
!      * use PGSTAT_RETRY_DELAY (since the number of worker is low, this
!      * shouldn't be a problem).
       *
       * Note that we don't recompute min_ts after sleeping; so we might end up
       * accepting a file a bit older than PGSTAT_STAT_INTERVAL.  In practice
--- 3388,3395 ----
       * We set the minimum acceptable timestamp to PGSTAT_STAT_INTERVAL msec
       * before now.  This indirectly ensures that the collector needn't write
       * the file more often than PGSTAT_STAT_INTERVAL.  In an autovacuum
!      * worker, however, we can reuse the stats file that was requested by the
!      * launcher.
       *
       * Note that we don't recompute min_ts after sleeping; so we might end up
       * accepting a file a bit older than PGSTAT_STAT_INTERVAL.  In practice
***************
*** 3400,3406 **** backend_read_statsfile(void)
       */
      if (IsAutoVacuumWorkerProcess())
          min_ts = TimestampTzPlusMilliseconds(GetCurrentTimestamp(),
!                                              -PGSTAT_RETRY_DELAY);
      else
          min_ts = TimestampTzPlusMilliseconds(GetCurrentTimestamp(),
                                               -PGSTAT_STAT_INTERVAL);
--- 3399,3405 ----
       */
      if (IsAutoVacuumWorkerProcess())
          min_ts = TimestampTzPlusMilliseconds(GetCurrentTimestamp(),
!                                              -autovacuum_naptime * 1000);
      else
          min_ts = TimestampTzPlusMilliseconds(GetCurrentTimestamp(),
                                               -PGSTAT_STAT_INTERVAL);