Re: Should vacuum process config file reload more often

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