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

On Wed, Mar 15, 2023 at 1:14 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Sat, Mar 11, 2023 at 8:11 AM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> > I've implemented the atomic cost limit in the attached patch. Though,
> > I'm pretty unsure about how I initialized the atomics in
> > AutoVacuumShmemInit()...
>
> +
>                  /* initialize the WorkerInfo free list */
>                  for (i = 0; i < autovacuum_max_workers; i++)
>                          dlist_push_head(&AutoVacuumShmem->av_freeWorkers,
>                                                          &worker[i].wi_links);
> +
> +                dlist_foreach(iter, &AutoVacuumShmem->av_freeWorkers)
> +                        pg_atomic_init_u32(
> +
> &(dlist_container(WorkerInfoData, wi_links, iter.cur))->wi_cost_limit,
> +                                                           0);
> +
>
> I think we can do like:
>
>     /* initialize the WorkerInfo free list */
>     for (i = 0; i < autovacuum_max_workers; i++)
>     {
>         dlist_push_head(&AutoVacuumShmem->av_freeWorkers,
>                                         &worker[i].wi_links);
>         pg_atomic_init_u32(&(worker[i].wi_cost_limit));
>     }

Ah, yes, I was distracted by the variable name "worker" (as opposed to
"workers").

> > 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.
> >
>
> If we remove wi_cost_delay from WorkerInfo, probably we don't need to
> acquire the lwlock in AutoVacuumUpdateDelay()? The shared field we
> access in that function will be only wi_dobalance, but this field is
> updated only by its owner autovacuum worker.

I realized that we cannot use dobalance to decide whether or not to
update wi_cost_delay because dobalance could be false because of table
option cost limit being set (with no table option cost delay) and we
would still need to update VacuumCostDelay and wi_cost_delay with the
new value of autovacuum_vacuum_cost_delay.

But v5 skirts around this issue altogether.

> > > ---
> > >  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().
>
> But doesn't the launcher do a similar thing at the beginning of
> autovac_balance_cost()?
>
>     double      vac_cost_delay = (autovacuum_vac_cost_delay >= 0 ?
>                                   autovacuum_vac_cost_delay : VacuumCostDelay);

Ah, yes. You are right.

> Related to this point, I think autovac_balance_cost() should use
> globally-set cost_limit and cost_delay values to calculate worker's
> vacuum-delay parameters. IOW, vac_cost_limit and vac_cost_delay should
> come from the config file setting, not table option etc:
>
>     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);
>
> If my understanding is right, the following change is not right;
> AutoVacUpdateLimit() updates the VacuumCostLimit based on the value in
> MyWorkerInfo:
>
>                  MyWorkerInfo->wi_cost_limit_base = tab->at_vacuum_cost_limit;
> +                AutoVacuumUpdateLimit();
>
>                  /* do a balance */
>                  autovac_balance_cost();
>
> -                /* set the active cost parameters from the result of that */
> -                AutoVacuumUpdateDelay();
>
> Also, even when using the global variables for cost delay, the
> launcher doesn't need to check the global variable. It should always
> be able to use either autovacuum_vac_cost_delay/limit or
> VacuumCostDelay/Limit.

Yes, that is true. But, I actually think we can do something more
radical, which relates to this point as well as the issue with
cost_limit_base below.

> > 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?
>
> Yeah, the current balancing algorithm looks to respect the cost_limit
> value set when starting to vacuum the table. The proportion of the
> amount of I/O that a worker can consume is calculated based on the
> base value and the new worker's cost_limit value cannot exceed the
> base value. Given that we're trying to dynamically tune worker's cost
> parameters (delay and limit), this concept seems to need to be
> updated.

In master, autovacuum workers reload the config file at most once per
table vacuumed. And that is the same time that they update their
wi_cost_limit_base and wi_cost_delay. Thus, when autovac_balance_cost()
is called, there is a good chance that different workers will have
different values for wi_cost_limit_base and wi_cost_delay (and we are
only talking about workers not vacuuming a table with table option
cost-related gucs). So, it made sense that the balancing algorithm tried
to use a ratio to determine what to set the cost limit of each worker
to. It is clamped to the base value, as you say, but it also gives
workers a proportion of the new limit equal to what proportion their base
cost represents of the total cost.

I think all of this doesn't matter anymore now that everyone can reload
the config file often and dynamically change these values.

Thus, in the attached v5, I have removed both wi_cost_limit and wi_cost_delay
from WorkerInfo. I've added a new variable to AutoVacuumShmem called
nworkers_for_balance. Now, autovac_balance_cost() only recalculates this
number and updates it if it has changed. Then, in
AutoVacuumUpdateLimit() workers read from this atomic value and divide
the value of the cost limit gucs by that number to get their own cost limit.

I keep the table option value of cost limit and cost delay in
backend-local memory to reference when updating the worker cost limit.

One nice thing is autovac_balance_cost() only requires an access shared
lock now (though most callers are updating other members before calling
it and still take an exclusive lock).

What do you think?

> > > > 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.
>
> Indeed. But does it mean that there is no code path to turn
> vacuum-delay on, even when vacuum_cost_delay is updated from 0 to
> non-0?

Ah yes! Good point. This is true.
I'm not sure how to cheaply allow for re-enabling delays after disabling
them in the middle of a table vacuum.

I don't see a way around checking if we need to reload the config file
on every call to vacuum_delay_point() (currently, we are only doing this
when we have to wait anyway). It seems expensive to do this check every
time. If we do do this, we would update VacuumCostActive when updating
VacuumCostDelay, and we would need a global variable keeping the
failsafe status, as you mentioned.

It could be okay to say that you can only disable cost-based delays in
the middle of vacuuming a table (i.e. you cannot enable them if they are
already disabled until you start vacuuming the next table). Though maybe
it is weird that you can increase the delay but not re-enable it...

On an unrelated note, I was wondering if there were any docs anywhere
that should be updated to go along with this.

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.

- Melanie