Re: Should vacuum process config file reload more often

On Sat, Mar 25, 2023 at 3:03 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> On Thu, Mar 23, 2023 at 8:27 PM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> > On Thu, Mar 23, 2023 at 2:09 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > > And, I was wondering if it was worth trying to split up the part that
> > > > reloads the config file and all of the autovacuum stuff. The reloading
> > > > of the config file by itself won't actually result in autovacuum workers
> > > > having updated cost delays because of them overwriting it with
> > > > wi_cost_delay, but it will allow VACUUM to have those updated values.
> > >
> > > It makes sense to me to have changes for overhauling the rebalance
> > > mechanism in a separate patch.
> > >
> > > Looking back at the original concern you mentioned[1]:
> > >
> > > speed up long-running vacuum of a large table by
> > > decreasing autovacuum_vacuum_cost_delay/vacuum_cost_delay, however the
> > > config file is only reloaded between tables (for autovacuum) or after
> > > the statement (for explicit vacuum).
> > >
> > > does it make sense to have autovac_balance_cost() update workers'
> > > wi_cost_delay too? Autovacuum launcher already reloads the config file
> > > and does the rebalance. So I thought autovac_balance_cost() can update
> > > the cost_delay as well, and this might be a minimal change to deal
> > > with your concern. This doesn't have the effect for manual VACUUM but
> > > since vacuum delay is disabled by default it won't be a big problem.
> > > As for manual VACUUMs, we would need to reload the config file in
> > > vacuum_delay_point() as the part of your patch does. Overhauling the
> > > rebalance mechanism would be another patch to improve it further.
> >
> > So, we can't do this without acquiring an access shared lock on every
> > call to vacuum_delay_point() because cost delay is a double.
> >
> > I will work on a patchset with separate commits for reloading the config
> > file, though (with autovac not benefitting in the first commit).
>
> So, I realized we could actually do as you say and have autovac workers
> update their wi_cost_delay and keep the balance changes in a separate
> commit. I've done this in attached v8.
>
> Workers take the exclusive lock to update their wi_cost_delay and
> wi_cost_limit only when there is a config reload. So, there is one
> commit that implements this behavior and a separate commit to revise the
> worker rebalancing.

So, I've attached an alternate version of the patchset which takes the
approach of having one commit which only enables cost-based delay GUC
refresh for VACUUM and another commit which enables it for autovacuum
and makes the changes to balancing variables.

I still think the commit which has workers updating their own
wi_cost_delay in vacuum_delay_point() is a bit weird. It relies on no one
else emulating our bad behavior and reading from wi_cost_delay without a
lock and on no one else deciding to ever write to wi_cost_delay (even
though it is in shared memory [this is the same as master]). It is only
safe because our process is the only one (right now) writing to
wi_cost_delay, so when we read from it without a lock, we know it isn't
being written to. And everyone else takes a lock when reading from
wi_cost_delay right now. So, it seems...not great.

This approach also introduces a function that is only around for
one commit until the next commit obsoletes it, which seems a bit silly.

Basically, I think it is probably better to just have one commit
enabling guc refresh for VACUUM and then another which correctly
implements what is needed for autovacuum to do the same.
Attached v9 does this.

I've provided both complete versions of both approaches (v9 and v8).

- Melanie