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

On Sun, Mar 19, 2023 at 7:47 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> 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?

I think this is a good idea.

Do we need to calculate the number of workers running with
nworkers_for_balance by iterating over the running worker list? I
guess autovacuum workers can increment/decrement it at the beginning
and end of vacuum.

>
> > > > > 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 Mon, Mar 20, 2023 at 1:48 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
> So, I thought about it some more, and I think it is a bit odd that you
> can increase the delay and limit but not re-enable them if they were
> disabled. And, perhaps it would be okay to check ConfigReloadPending at
> the top of vacuum_delay_point() instead of only after sleeping. It is
> just one more branch. We can check if VacuumCostActive is false after
> checking if we should reload and doing so if needed and return early.
> I've implemented that in attached v6.
>
> I added in the global we discussed for VacuumFailsafeActive. If we keep
> it, we can probably remove the one in LVRelState -- as it seems
> redundant. Let me know what you think.

I think the following change is related:

-        if (!VacuumCostActive || InterruptPending)
+        if (InterruptPending || VacuumFailsafeActive ||
+                (!VacuumCostActive && !ConfigReloadPending))
                 return;

+        /*
+         * Reload the configuration file if requested. This allows changes to
+         * [autovacuum_]vacuum_cost_limit and [autovacuum_]vacuum_cost_delay to
+         * take effect while a table is being vacuumed or analyzed.
+         */
+        if (ConfigReloadPending && !analyze_in_outer_xact)
+        {
+                ConfigReloadPending = false;
+                ProcessConfigFile(PGC_SIGHUP);
+                AutoVacuumUpdateDelay();
+                AutoVacuumUpdateLimit();
+        }

It makes sense to me that we need to reload the config file even when
vacuum-delay is disabled. But I think it's not convenient for users
that we don't reload the configuration file once the failsafe is
triggered. I think users might want to change some GUCs such as
log_autovacuum_min_duration.

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

The current patch improves the internal mechanism of (re)balancing
vacuum-cost but doesn't change user-visible behavior. I don't have any
idea so far that we should update somewhere in the doc.

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

Regards,

[1] https://www.postgresql.org/message-id/CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7%2BDXhrjDA0gw%40mail.gmail.com

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com