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

Quotes below are combined from two of Sawada-san's emails.

I've also attached a patch with my suggested current version.

On Thu, Mar 9, 2023 at 10:27 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Fri, Mar 10, 2023 at 11:23 AM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> >
> > 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().
>
> IIUC if any of the cost delay parameters has been set individually,
> the autovacuum worker is excluded from the balance algorithm.

Ah, yes! That's right. So it is not a problem. Then I still think
removing wi_cost_delay from the worker info makes sense. wi_cost_delay
is a double and can't easily be accessed atomically the way
wi_cost_limit can be.

Keeping the cost delay local to the backends also makes it clear that
cost delay is not something that should be written to by other backends
or that can differ from worker to worker. Without table options in the
picture, the cost delay should be the same for any worker who has
reloaded the config file.

As for the cost limit safe access issue, maybe we can avoid a LWLock
acquisition for reading wi_cost_limit by using an atomic similar to what
you suggested here for "did_rebalance".

> > 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.

Instead of having the atomic indicate whether or not someone (launcher
or another worker) did a rebalance, it would simply store the current
cost limit. Then the worker can normally access it with a simple read.

My rationale is that if we used an atomic to indicate whether or not we
did a rebalance ("did_rebalance"), we would have the same cache
coherency guarantees as if we just used the atomic for the cost limit.
If we read from the "did_rebalance" variable and missed someone having
written to it on another core, we still wouldn't get around to checking
the wi_cost_limit variable in shared memory, so it doesn't matter that
we bothered to keep it in shared memory and use a lock to access it.

I noticed we don't allow wi_cost_limit to ever be less than 0, so we
could store wi_cost_limit in an atomic uint32.

I'm not sure if it is okay to do pg_atomic_read_u32() and
pg_atomic_unlocked_write_u32() or if we need pg_atomic_write_u32() in
most cases.

I've implemented the atomic cost limit in the attached patch. Though,
I'm pretty unsure about how I initialized the atomics in
AutoVacuumShmemInit()...

If the consensus is that it is simply too confusing to take
wi_cost_delay out of WorkerInfo, we might be able to afford using a
shared lock to access it because we won't call AutoVacuumUpdateDelay()
on every invocation of vacuum_delay_point() -- only when we've reloaded
the config file.

One potential option to avoid taking a shared lock on every call to
AutoVacuumUpdateDelay() is to set a global variable to indicate that we
did update it (since we are the only ones updating it) and then only
take the shared LWLock in AutoVacuumUpdateDelay() if that flag is true.

> ---
>  void
>  AutoVacuumUpdateDelay(void)
>  {
> -        if (MyWorkerInfo)
> +        /*
> +         * We are using autovacuum-related GUCs to update
> VacuumCostDelay, so we
> +         * only want autovacuum workers and autovacuum launcher to do this.
> +         */
> +        if (!(am_autovacuum_worker || am_autovacuum_launcher))
> +                return;
>
> Is there any case where the autovacuum launcher calls
> AutoVacuumUpdateDelay() function?

I had meant to add it to HandleAutoVacLauncherInterrupts() after
reloading the config file (done in attached patch). When using the
global variables for cost delay (instead of wi_cost_delay in worker
info), the autovac launcher also has to do the check in the else branch
of AutoVacuumUpdateDelay()

        VacuumCostDelay = autovacuum_vac_cost_delay >= 0 ?
            autovacuum_vac_cost_delay : VacuumCostDelay;

to make sure VacuumCostDelay is correct for when it calls
autovac_balance_cost().

This also made me think about whether or not we still need cost_limit_base.
It is used to ensure that autovac_balance_cost() never ends up setting
workers' wi_cost_limits above the current autovacuum_vacuum_cost_limit
(or VacuumCostLimit). However, the launcher and all the workers should
know what the value is without cost_limit_base, no?

> ---
> In at autovac_balance_cost(), we have,
>
>     int         vac_cost_limit = (autovacuum_vac_cost_limit > 0 ?
>                                   autovacuum_vac_cost_limit : VacuumCostLimit);
>     double      vac_cost_delay = (autovacuum_vac_cost_delay >= 0 ?
>                                   autovacuum_vac_cost_delay : VacuumCostDelay);
>     :
>     /* not set? nothing to do */
>     if (vac_cost_limit <= 0 || vac_cost_delay <= 0)
>         return;
>
> IIUC if autovacuum_vac_cost_delay is changed to 0 during autovacuums
> running, their vacuum delay parameters are not changed. It's not a bug
> of the patch but I think we can fix it in this patch.

Yes, currently (in master) wi_cost_delay does not get updated anywhere.
In my patch, the global variable we are using for delay is updated but
it is not done in autovac_balance_cost().

> > Also not sure how the patch interacts with failsafe autovac and parallel
> > vacuum.
>
> Good point.
>
> When entering the failsafe mode, we disable the vacuum delays (see
> lazy_check_wraparound_failsafe()). We need to keep disabling the
> vacuum delays even after reloading the config file. One idea is to
> have another global variable indicating we're in the failsafe mode.
> vacuum_delay_point() doesn't update VacuumCostActive if the flag is
> true.

I think we might not need to do this. Other than in
lazy_check_wraparound_failsafe(), VacuumCostActive is only updated in
two places:

1) in vacuum() which autovacuum will call per table. And failsafe is
reset per table as well.

2) in vacuum_delay_point(), but, since VacuumCostActive will already be
false when we enter vacuum_delay_point() the next time after
lazy_check_wraparound_failsafe(), we won't set VacuumCostActive there.

Thanks again for the detailed feedback!

- Melanie