Fix leakage of cost_limit when multiple autovacuum workers are active.

When using default autovacuum_vac_cost_limit, autovac_balance_cost relied
on VacuumCostLimit to contain the correct global value ... but after the
first time through in a particular worker process, it didn't, because we'd
trashed it in previous iterations.  Depending on the state of other autovac
workers, this could result in a steady reduction of the effective
cost_limit setting as a particular worker processed more and more tables,
causing it to go slower and slower.  Spotted by Simon Poole (bug #5759).
Fix by saving and restoring the GUC variables in the loop in do_autovacuum.

In passing, improve a few comments.

Back-patch to 8.3 ... the cost rebalancing code has been buggy since it was
put in.

diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index a617b882a34a286fbcee0c6526cc09fdb3bd9628..c7d704dc8c80253698a882cbccef4227a92963a7 100644 (file)
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -190,7 +190,7 @@ typedef struct autovac_table
  *
  * wi_links        entry into free list or running list
  * wi_dboid        OID of the database this worker is supposed to work on
- * wi_tableoid OID of the table currently being vacuumed
+ * wi_tableoid OID of the table currently being vacuumed, if any
  * wi_proc     pointer to PGPROC of the running worker, NULL if not started
  * wi_launchtime Time at which this worker was launched
  * wi_cost_*   Vacuum cost-based delay parameters current in this worker
@@ -1629,7 +1629,7 @@ FreeWorkerInfo(int code, Datum arg)
         * limit setting of the remaining workers.
         *
         * We somewhat ignore the risk that the launcher changes its PID
-        * between we reading it and the actual kill; we expect ProcKill to be
+        * between us reading it and the actual kill; we expect ProcKill to be
         * called shortly after us, and we assume that PIDs are not reused too
         * quickly after a process exits.
         */
@@ -1673,16 +1673,18 @@ AutoVacuumUpdateDelay(void)
 
 /*
  * autovac_balance_cost
- *     Recalculate the cost limit setting for each active workers.
+ *     Recalculate the cost limit setting for each active worker.
  *
  * Caller must hold the AutovacuumLock in exclusive mode.
  */
 static void
 autovac_balance_cost(void)
 {
-   WorkerInfo  worker;
-
    /*
+    * The idea here is that we ration out I/O equally.  The amount of I/O
+    * that a worker can consume is determined by cost_limit/cost_delay, so
+    * we try to equalize those ratios rather than the raw limit settings.
+    *
     * note: in cost_limit, zero also means use value from elsewhere, because
     * zero is not a valid value.
     */
@@ -1692,6 +1694,7 @@ autovac_balance_cost(void)
                                autovacuum_vac_cost_delay : VacuumCostDelay);
    double      cost_total;
    double      cost_avail;
+   WorkerInfo  worker;
 
    /* not set? nothing to do */
    if (vac_cost_limit <= 0 || vac_cost_delay <= 0)
@@ -1718,7 +1721,7 @@ autovac_balance_cost(void)
        return;
 
    /*
-    * Adjust each cost limit of active workers to balance the total of cost
+    * Adjust cost limit of each active worker to balance the total of cost
     * limit to autovacuum_vacuum_cost_limit.
     */
    cost_avail = (double) vac_cost_limit / vac_cost_delay;
@@ -1734,14 +1737,19 @@ autovac_balance_cost(void)
            (cost_avail * worker->wi_cost_limit_base / cost_total);
 
            /*
-            * We put a lower bound of 1 to the cost_limit, to avoid division-
-            * by-zero in the vacuum code.
+            * We put a lower bound of 1 on the cost_limit, to avoid division-
+            * by-zero in the vacuum code.  Also, in case of roundoff trouble
+            * in these calculations, let's be sure we don't ever set
+            * cost_limit to more than the base value.
             */
-           worker->wi_cost_limit = Max(Min(limit, worker->wi_cost_limit_base), 1);
-
-           elog(DEBUG2, "autovac_balance_cost(pid=%u db=%u, rel=%u, cost_limit=%d, cost_delay=%d)",
-                worker->wi_proc->pid, worker->wi_dboid,
-                worker->wi_tableoid, worker->wi_cost_limit, worker->wi_cost_delay);
+           worker->wi_cost_limit = Max(Min(limit,
+                                           worker->wi_cost_limit_base),
+                                       1);
+
+           elog(DEBUG2, "autovac_balance_cost(pid=%u db=%u, rel=%u, cost_limit=%d, cost_limit_base=%d, cost_delay=%d)",
+                worker->wi_proc->pid, worker->wi_dboid, worker->wi_tableoid,
+                worker->wi_cost_limit, worker->wi_cost_limit_base,
+                worker->wi_cost_delay);
        }
 
        worker = (WorkerInfo) SHMQueueNext(&AutoVacuumShmem->av_runningWorkers,
@@ -2125,6 +2133,8 @@ do_autovacuum(void)
        autovac_table *tab;
        WorkerInfo  worker;
        bool        skipit;
+       int         stdVacuumCostDelay;
+       int         stdVacuumCostLimit;
 
        CHECK_FOR_INTERRUPTS();
 
@@ -2198,11 +2208,15 @@ do_autovacuum(void)
        MyWorkerInfo->wi_tableoid = relid;
        LWLockRelease(AutovacuumScheduleLock);
 
-       /* Set the initial vacuum cost parameters for this table */
-       VacuumCostDelay = tab->at_vacuum_cost_delay;
-       VacuumCostLimit = tab->at_vacuum_cost_limit;
+       /*
+        * Remember the prevailing values of the vacuum cost GUCs.  We have
+        * to restore these at the bottom of the loop, else we'll compute
+        * wrong values in the next iteration of autovac_balance_cost().
+        */
+       stdVacuumCostDelay = VacuumCostDelay;
+       stdVacuumCostLimit = VacuumCostLimit;
 
-       /* Last fixups before actually starting to work */
+       /* Must hold AutovacuumLock while mucking with cost balance info */
        LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
 
        /* advertise my cost delay parameters for the balancing algorithm */
@@ -2213,6 +2227,9 @@ do_autovacuum(void)
        /* do a balance */
        autovac_balance_cost();
 
+       /* set the active cost parameters from the result of that */
+       AutoVacuumUpdateDelay();
+
        /* done */
        LWLockRelease(AutovacuumLock);
 
@@ -2290,10 +2307,20 @@ deleted:
            pfree(tab->at_relname);
        pfree(tab);
 
-       /* remove my info from shared memory */
+       /*
+        * Remove my info from shared memory.  We could, but intentionally
+        * don't, clear wi_cost_limit and friends --- this is on the
+        * assumption that we probably have more to do with similar cost
+        * settings, so we don't want to give up our share of I/O for a very
+        * short interval and thereby thrash the global balance.
+        */
        LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
        MyWorkerInfo->wi_tableoid = InvalidOid;
        LWLockRelease(AutovacuumLock);
+
+       /* restore vacuum cost GUCs for the next iteration */
+       VacuumCostDelay = stdVacuumCostDelay;
+       VacuumCostLimit = stdVacuumCostLimit;
    }
 
    /*