Re: Should vacuum process config file reload more often - Mailing list pgsql-hackers

Hi.

About 0001:

+ * VacuumFailsafeActive is a defined as a global so that we can determine
+ * whether or not to re-enable cost-based vacuum delay when vacuuming a table.
+ * If failsafe mode has been engaged, we will not re-enable cost-based delay
+ * for the table until after vacuuming has completed, regardless of other
+ * settings. Only VACUUM code should inspect this variable and only table
+ * access methods should set it. In Table AM-agnostic VACUUM code, this
+ * variable controls whether or not to allow cost-based delays. Table AMs are
+ * free to use it if they desire this behavior.
+ */
+bool        VacuumFailsafeActive = false;

If I understand this correctly, there seems to be an issue. The
AM-agnostic VACUUM code is setting it and no table AMs actually do
that.


0003:
+
+            /*
+             * Ensure VacuumFailsafeActive has been reset before vacuuming the
+             * next relation.
+             */
+            VacuumFailsafeActive = false;
         }
     }
     PG_FINALLY();
     {
         in_vacuum = false;
         VacuumCostActive = false;
+        VacuumFailsafeActive = false;
+        VacuumCostBalance = 0;

There is no need to reset VacuumFailsafeActive in the PG_TRY() block.


+    /*
+     * Reload the configuration file if requested. This allows changes to
+     * autovacuum_vacuum_cost_limit and autovacuum_vacuum_cost_delay to take
+     * effect while a table is being vacuumed or analyzed.
+     */
+    if (ConfigReloadPending && IsAutoVacuumWorkerProcess())
+    {
+        ConfigReloadPending = false;
+        ProcessConfigFile(PGC_SIGHUP);
+        VacuumUpdateCosts();
+    }

I believe we should prevent unnecessary reloading when
VacuumFailsafeActive is true.


+        AutoVacuumUpdateLimit();

I'm not entirely sure, but it might be better to name this
AutoVacuumUpdateCostLimit().


+    pg_atomic_flag wi_dobalance;
...
+        /*
+         * We only expect this worker to ever set the flag, so don't bother
+         * checking the return value. We shouldn't have to retry.
+         */
+        if (tab->at_dobalance)
+            pg_atomic_test_set_flag(&MyWorkerInfo->wi_dobalance);
+        else
+            pg_atomic_clear_flag(&MyWorkerInfo->wi_dobalance);

        LWLockAcquire(AutovacuumLock, LW_SHARED);

        autovac_recalculate_workers_for_balance();

I don't see the need for using atomic here. The code is executed
infrequently and we already take a lock while counting do_balance
workers. So sticking with the old locking method (taking LW_EXCLUSIVE
then set wi_dobalance then do balance) should be fine.


+void
+AutoVacuumUpdateLimit(void)
...
+    if (av_relopt_cost_limit > 0)
+        VacuumCostLimit = av_relopt_cost_limit;
+    else

I think we should use wi_dobalance to decide if we need to do balance
or not. We don't need to take a lock to do that since only the process
updates it.


/*
          * 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.
+         * don't, unset wi_dobalance on the assumption that we are more likely
+         * than not to vacuum a table with no table options next, 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(AutovacuumScheduleLock, LW_EXCLUSIVE);
         MyWorkerInfo->wi_tableoid = InvalidOid;

The comment mentions wi_dobalance, but it doesn't appear here..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center