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