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

On Tue, Mar 7, 2023 at 12:10 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Mar 6, 2023 at 5:26 AM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> >
> > On Thu, Mar 2, 2023 at 6:37 PM Melanie Plageman
> > 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.
>
> While it's true that wi_cost_delay doesn't need to be shared, it seems
> to make the logic somewhat complex. We need to handle cost_delay in a
> different way from other vacuum-related parameters and we need to make
> sure av[_use]_table_option_cost_delay are set properly. Removing
> wi_cost_delay from WorkerInfoData saves 8 bytes shared memory per
> autovacuum worker but it might be worth considering to keep
> wi_cost_delay for simplicity.

Ah, it turns out we can't really remove wi_cost_delay from WorkerInfo
anyway because the launcher doesn't know anything about table options
and so the workers have to keep an updated wi_cost_delay that the
launcher or other autovac workers who are not vacuuming that table can
read from when calculating the new limit in autovac_balance_cost().

However, wi_cost_delay is a double, so if we start updating it on config
reload in vacuum_delay_point(), we definitely need some protection
against torn reads.

The table options can only change when workers start vacuuming a new
table, so maybe there is some way to use this to solve this problem?

> > 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.
>
> Indeed.
>
> > 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?
>
> Yeah, acquiring the lwlock for every call to vacuum_delay_point()
> seems to be harmful. One idea would be to have one sig_atomic_t
> variable in WorkerInfoData and autovac_balance_cost() set it to true
> after rebalancing the worker's cost-limit. The worker can check it
> without locking and update its delay parameters if the flag is true.

Maybe we can do something like this with the table options values?

- Melanie