Re: Should vacuum process config file reload more often

On Thu, Mar 2, 2023 at 6:37 PM Melanie Plageman
<melanieplageman(at)gmail(dot)com> wrote:
>
> On Thu, Mar 2, 2023 at 2:36 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Thu, Mar 2, 2023 at 10:41 AM Melanie Plageman
> > <melanieplageman(at)gmail(dot)com> wrote:
> > > On another topic, I've just realized that when autovacuuming we only
> > > update tab->at_vacuum_cost_delay/limit from
> > > autovacuum_vacuum_cost_delay/limit for each table (in
> > > table_recheck_autovac()) and then use that to update
> > > MyWorkerInfo->wi_cost_delay/limit. MyWorkerInfo->wi_cost_delay/limit is
> > > what is used to update VacuumCostDelay/Limit in AutoVacuumUpdateDelay().
> > > So, even if we reload the config file in vacuum_delay_point(), if we
> > > don't use the new value of autovacuum_vacuum_cost_delay/limit it will
> > > have no effect for autovacuum.
> >
> > Right, but IIUC wi_cost_limit (and VacuumCostDelayLimit) might be
> > updated. After the autovacuum launcher reloads the config file, it
> > calls autovac_balance_cost() that updates that value of active
> > workers. I'm not sure why we don't update workers' wi_cost_delay,
> > though.
>
> Ah yes, I didn't realize this. Thanks. I went back and did more code
> reading/analysis, and I see no reason why we shouldn't update
> worker->wi_cost_delay to the new value of autovacuum_vac_cost_delay in
> autovac_balance_cost(). Then, as you said, the autovac launcher will
> call autovac_balance_cost() when it reloads the configuration file.
> Then, the next time the autovac worker calls AutoVacuumUpdateDelay(), it
> will update VacuumCostDelay.
>
> > > I started writing a little helper that could be used to update these
> > > workerinfo->wi_cost_delay/limit in vacuum_delay_point(),
> >
> > Since we set vacuum delay parameters for autovacuum workers so that we
> > ration out I/O equally, I think we should keep the current mechanism
> > that the autovacuum launcher sets workers' delay parameters and they
> > update accordingly.
>
> Yes, agreed, it should go in the same place as where we update
> wi_cost_limit (autovac_balance_cost()). I think we should potentially
> rename autovac_balance_cost() because its name and all its comments
> point to its only purpose being to balance the total of the workers
> wi_cost_limits to no more than autovacuum_vacuum_cost_limit. And the
> autovacuum_vacuum_cost_delay doesn't need to be balanced in this way.
>
> Though, since this change on its own would make autovacuum pick up new
> values of autovacuum_vacuum_cost_limit (without having the worker reload
> the config file), I wonder if it makes sense to try and have
> vacuum_delay_point() only reload the config file if it is an explicit
> vacuum or an analyze not being run in an outer transaction (to avoid
> overhead of reloading config file)?
>
> The lifecycle of this different vacuum delay-related gucs and how it
> differs between autovacuum workers and explicit vacuum is quite tangled
> already, though.

So, I've attached a new version of the patch which is quite different
from the previous versions.

In this version I've removed wi_cost_delay from WorkerInfoData. There is
no synchronization of cost_delay amongst workers, so there is no reason
to keep it in shared memory.

One consequence of not updating VacuumCostDelay from wi_cost_delay is
that we have to have a way to keep track of whether or not autovacuum
table options are in use.

This patch does this in a cringeworthy way. I added two global
variables, one to track whether or not cost delay table options are in
use and the other to store the value of the table option cost delay. I
didn't want to use a single variable with a special value to indicate
that table option cost delay is in use because
autovacuum_vacuum_cost_delay already has special values that mean
certain things. My code needs a better solution.

It is worth mentioning that I think that in master,
AutoVacuumUpdateDelay() was incorrectly reading wi_cost_limit and
wi_cost_delay from shared memory without holding a lock.

I've added in a shared lock for reading from wi_cost_limit in this
patch. However, AutoVacuumUpdateLimit() is called unconditionally in
vacuum_delay_point(), which is called quite often (per block-ish), so I
was trying to think if there is a way we could avoid having to check
this shared memory variable on every call to vacuum_delay_point().
Rebalances shouldn't happen very often (done by the launcher when a new
worker is launched and by workers between vacuuming tables). Maybe we
can read from it less frequently?

Also not sure how the patch interacts with failsafe autovac and parallel
vacuum.

- Melanie