Re: Should vacuum process config file reload more often

I think attached v18 addresses all outstanding issues except a run
through the docs making sure all mentions of the balancing algorithm are
still correct.

On Wed, Apr 5, 2023 at 9:10 AM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> > On 4 Apr 2023, at 22:04, Melanie Plageman <melanieplageman(at)gmail(dot)com> wrote:
> >> +extern int VacuumCostLimit;
> >> +extern double VacuumCostDelay;
> >> ...
> >> -extern PGDLLIMPORT int VacuumCostLimit;
> >> -extern PGDLLIMPORT double VacuumCostDelay;
> >>
> >> Same with these, I don't think this is according to our default visibility.
> >> Moreover, I'm not sure it's a good idea to perform this rename. This will keep
> >> VacuumCostLimit and VacuumCostDelay exported, but change their meaning. Any
> >> external code referring to these thinking they are backing the GUCs will still
> >> compile, but may be broken in subtle ways. Is there a reason for not keeping
> >> the current GUC variables and instead add net new ones?
> >
> > When VacuumCostLimit was the same variable in the code and for the GUC
> > vacuum_cost_limit, everytime we reload the config file, VacuumCostLimit
> > is overwritten. Autovacuum workers have to overwrite this value with the
> > appropriate one for themselves given the balancing logic and the value
> > of autovacuum_vacuum_cost_limit. However, the problem is, because you
> > can specify -1 for autovacuum_vacuum_cost_limit to indicate it should
> > fall back to vacuum_cost_limit, we have to reference the value of
> > VacuumCostLimit when calculating the new autovacuum worker's cost limit
> > after a config reload.
> >
> > But, you have to be sure you *only* do this after a config reload when
> > the value of VacuumCostLimit is fresh and unmodified or you risk
> > dividing the value of VacuumCostLimit over and over. That means it is
> > unsafe to call functions updating the cost limit more than once.
> >
> > This orchestration wasn't as difficult when we only reloaded the config
> > file once every table. We were careful about it and also kept the
> > original "base" cost limit around from table_recheck_autovac(). However,
> > once we started reloading the config file more often, this no longer
> > works.
> >
> > By separating the variables modified when the gucs are set and the ones
> > used the code, we can make sure we always have the original value the
> > guc was set to in vacuum_cost_limit and autovacuum_vacuum_cost_limit,
> > whenever we need to reference it.
> >
> > That being said, perhaps we should document what extensions should do?
> > Do you think they will want to use the variables backing the gucs or to
> > be able to overwrite the variables being used in the code?
>
> I think I wasn't clear in my comment, sorry. I don't have a problem with
> introducing a new variable to split the balanced value from the GUC value.
> What I don't think we should do is repurpose an exported symbol into doing a
> new thing. In the case at hand I think VacuumCostLimit and VacuumCostDelay
> should remain the backing variables for the GUCs, with vacuum_cost_limit and
> vacuum_cost_delay carrying the balanced values. So the inverse of what is in
> the patch now.
>
> The risk of these symbols being used in extensions might be very low but on
> principle it seems unwise to alter a symbol and risk subtle breakage.

In attached v18, I have flipped them. Existing (in master) GUCs which
were exported for VacuumCostLimit and VacuumCostDelay retain their names
and new globals vacuum_cost_limit and vacuum_cost_delay have been
introduced for use in the code.

Flipping these kind of melted my mind, so I could definitely use another
set of eyes double checking that the correct ones are being used in the
correct places throughout 0002 and 0003.

On Thu, Apr 6, 2023 at 3:09 PM Melanie Plageman
<melanieplageman(at)gmail(dot)com> wrote:
>
> v17 attached does not yet fix the logging problem or variable naming
> problem.
>
> I have not changed where AutoVacuumUpdateCostLimit() is called either.
>
> This is effectively just a round of cleanup. I hope I have managed to
> address all other code review feedback so far, though some may have
> slipped through the cracks.
>
> On Wed, Apr 5, 2023 at 2:56 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Wed, Apr 5, 2023 at 11:29 AM Melanie Plageman <melanieplageman(at)gmail(dot)com> wrote:
> > + /*
> > + * Balance and update limit values for autovacuum workers. We must
> > + * always do this in case the autovacuum launcher or another
> > + * autovacuum worker has recalculated the number of workers across
> > + * which we must balance the limit. This is done by the launcher when
> > + * launching a new worker and by workers before vacuuming each table.
> > + */
> >
> > I don't quite understand what's going on here. A big reason that I'm
> > worried about this whole issue in the first place is that sometimes
> > there's a vacuum going on a giant table and you can't get it to go
> > fast. You want it to absorb new settings, and to do so quickly. I
> > realize that this is about the number of workers, not the actual cost
> > limit, so that makes what I'm about to say less important. But ... is
> > this often enough? Like, the time before we move onto the next table
> > could be super long. The time before a new worker is launched should
> > be ~autovacuum_naptime/autovacuum_max_workers or ~20s with default
> > settings, so that's not horrible, but I'm kind of struggling to
> > understand the rationale for this particular choice. Maybe it's fine.
>
> I've at least updated this comment to be more correct/less misleading.
>
> >
> > + if (autovacuum_vac_cost_limit > 0)
> > + VacuumCostLimit = autovacuum_vac_cost_limit;
> > + else
> > + VacuumCostLimit = vacuum_cost_limit;
> > +
> > + /* Only balance limit if no cost-related storage
> > parameters specified */
> > + if (pg_atomic_unlocked_test_flag(&MyWorkerInfo->wi_dobalance))
> > + return;
> > + Assert(VacuumCostLimit > 0);
> > +
> > + nworkers_for_balance = pg_atomic_read_u32(
> > +
> > &AutoVacuumShmem->av_nworkersForBalance);
> > +
> > + /* There is at least 1 autovac worker (this worker). */
> > + if (nworkers_for_balance <= 0)
> > + elog(ERROR, "nworkers_for_balance must be > 0");
> > +
> > + VacuumCostLimit = Max(VacuumCostLimit /
> > nworkers_for_balance, 1);
> >
> > I think it would be better stylistically to use a temporary variable
> > here and only assign the final value to VacuumCostLimit.
>
> I tried that and thought it adding confusing clutter. If it is a code
> cleanliness issue, I am willing to change it, though.
>
> On Wed, Apr 5, 2023 at 3:04 PM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> >
> > > On 5 Apr 2023, at 20:55, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> >
> > > Again, I don't think this is something we should try to
> > > address right now under time pressure, but in the future, I think we
> > > should consider ripping this behavior out.
> >
> > I would not be opposed to that, but I wholeheartedly agree that it's not the
> > job of this patch (or any patch at this point in the cycle).
> >
> > > + if (autovacuum_vac_cost_limit > 0)
> > > + VacuumCostLimit = autovacuum_vac_cost_limit;
> > > + else
> > > + VacuumCostLimit = vacuum_cost_limit;
> > > +
> > > + /* Only balance limit if no cost-related storage
> > > parameters specified */
> > > + if (pg_atomic_unlocked_test_flag(&MyWorkerInfo->wi_dobalance))
> > > + return;
> > > + Assert(VacuumCostLimit > 0);
> > > +
> > > + nworkers_for_balance = pg_atomic_read_u32(
> > > +
> > > &AutoVacuumShmem->av_nworkersForBalance);
> > > +
> > > + /* There is at least 1 autovac worker (this worker). */
> > > + if (nworkers_for_balance <= 0)
> > > + elog(ERROR, "nworkers_for_balance must be > 0");
> > > +
> > > + VacuumCostLimit = Max(VacuumCostLimit /
> > > nworkers_for_balance, 1);
> > >
> > > I think it would be better stylistically to use a temporary variable
> > > here and only assign the final value to VacuumCostLimit.
> >
> > I can agree with that. Another supertiny nitpick on the above is to not end a
> > single-line comment with a period.
>
> I have fixed this.
>
> On Thu, Apr 6, 2023 at 2:40 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Thu, Apr 6, 2023 at 12:29 AM Melanie Plageman
> > <melanieplageman(at)gmail(dot)com> wrote:
> > >
> > > Thanks all for the reviews.
> > >
> > > v16 attached. I put it together rather quickly, so there might be a few
> > > spurious whitespaces or similar. There is one rather annoying pgindent
> > > outlier that I have to figure out what to do about as well.
> > >
> > > The remaining functional TODOs that I know of are:
> > >
> > > - Resolve what to do about names of GUC and vacuum variables for cost
> > > limit and cost delay (since it may affect extensions)
> > >
> > > - Figure out what to do about the logging message which accesses dboid
> > > and tableoid (lock/no lock, where to put it, etc)
> > >
> > > - I see several places in docs which reference the balancing algorithm
> > > for autovac workers. I did not read them in great detail, but we may
> > > want to review them to see if any require updates.
> > >
> > > - Consider whether or not the initial two commits should just be
> > > squashed with the third commit
> > >
> > > - Anything else reviewers are still unhappy with
> > >
> > >
> > > On Wed, Apr 5, 2023 at 1:56 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > > >
> > > > On Wed, Apr 5, 2023 at 5:05 AM Melanie Plageman
> > > > <melanieplageman(at)gmail(dot)com> wrote:
> > > > >
> > > > > On Tue, Apr 4, 2023 at 4:27 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > > > > > ---
> > > > > > - if (worker->wi_proc != NULL)
> > > > > > - elog(DEBUG2, "autovac_balance_cost(pid=%d
> > > > > > db=%u, rel=%u, dobalance=%s cost_limit=%d, cost_limit_base=%d,
> > > > > > cost_delay=%g)",
> > > > > > - worker->wi_proc->pid,
> > > > > > worker->wi_dboid, worker->wi_tableoid,
> > > > > > - worker->wi_dobalance ? "yes" : "no",
> > > > > > - worker->wi_cost_limit,
> > > > > > worker->wi_cost_limit_base,
> > > > > > - worker->wi_cost_delay);
> > > > > >
> > > > > > I think it's better to keep this kind of log in some form for
> > > > > > debugging. For example, we can show these values of autovacuum workers
> > > > > > in VacuumUpdateCosts().
> > > > >
> > > > > I added a message to do_autovacuum() after calling VacuumUpdateCosts()
> > > > > in the loop vacuuming each table. That means it will happen once per
> > > > > table. It's not ideal that I had to move the call to VacuumUpdateCosts()
> > > > > behind the shared lock in that loop so that we could access the pid and
> > > > > such in the logging message after updating the cost and delay, but it is
> > > > > probably okay. Though noone is going to be changing those at this
> > > > > point, it still seemed better to access them under the lock.
> > > > >
> > > > > This does mean we won't log anything when we do change the values of
> > > > > VacuumCostDelay and VacuumCostLimit while vacuuming a table. Is it worth
> > > > > adding some code to do that in VacuumUpdateCosts() (only when the value
> > > > > has changed not on every call to VacuumUpdateCosts())? Or perhaps we
> > > > > could add it in the config reload branch that is already in
> > > > > vacuum_delay_point()?
> > > >
> > > > Previously, we used to show the pid in the log since a worker/launcher
> > > > set other workers' delay costs. But now that the worker sets its delay
> > > > costs, we don't need to show the pid in the log. Also, I think it's
> > > > useful for debugging and investigating the system if we log it when
> > > > changing the values. The log I imagined to add was like:
> > > >
> > > > @@ -1801,6 +1801,13 @@ VacuumUpdateCosts(void)
> > > > VacuumCostDelay = vacuum_cost_delay;
> > > >
> > > > AutoVacuumUpdateLimit();
> > > > +
> > > > + elog(DEBUG2, "autovacuum update costs (db=%u, rel=%u,
> > > > dobalance=%s, cost_limit=%d, cost_delay=%g active=%s failsafe=%s)",
> > > > + MyWorkerInfo->wi_dboid, MyWorkerInfo->wi_tableoid,
> > > > + pg_atomic_unlocked_test_flag(&MyWorkerInfo->wi_dobalance)
> > > > ? "no" : "yes",
> > > > + VacuumCostLimit, VacuumCostDelay,
> > > > + VacuumCostDelay > 0 ? "yes" : "no",
> > > > + VacuumFailsafeActive ? "yes" : "no");
> > > > }
> > > > else
> > > > {
> > >
> > > Makes sense. I've updated the log message to roughly what you suggested.
> > > I also realized I think it does make sense to call it in
> > > VacuumUpdateCosts() -- only for autovacuum workers of course. I've done
> > > this. I haven't taken the lock though and can't decide if I must since
> > > they access dboid and tableoid -- those are not going to change at this
> > > point, but I still don't know if I can access them lock-free...
> > > Perhaps there is a way to condition it on the log level?
> > >
> > > If I have to take a lock, then I don't know if we should put these in
> > > VacuumUpdateCosts()...
> >
> > I think we don't need to acquire a lock there as both values are
> > updated only by workers reporting this message.
>
> I dunno. I just don't feel that comfortable saying, oh it's okay to
> access these without a lock probably. I propose we do one of the
> following:
>
> - Take a shared lock inside VacuumUpdateCosts() (it is not called on every
> call to vacuum_delay_point()) before reading from these variables.
>
> Pros:
> - We will log whenever there is a change to these parameters
> Cons:
> - This adds overhead in the common case when log level is < DEBUG2.
> Is there a way to check the log level before taking the lock?
> - Acquiring the lock inside the function is inconsistent with the
> pattern that some of the other autovacuum functions requiring
> locks use (they assume you have a lock if needed inside of the
> function). But, we could assert that the lock is not already held.
> - If we later decide we don't like this choice and want to move the
> logging elsewhere, it will necessarily log less frequently which
> seems like a harder change to make than logging more frequently.
>
> - Move this logging into the loop through relations in do_autovacuum()
> and the config reload code and take the shared lock before doing the
> logging.
>
> Pros:
> - Seems safe and not expensive
> - Covers most of the times we would want the logging
> Cons:
> - duplicates logging in two places

Okay, in an attempt to wrap up this saga, I have made the following
change:

Autovacuum workers, at the end of VacuumUpdateCosts(), check if cost
limit or cost delay have been changed. If they have, they assert that
they don't already hold the AutovacuumLock, take it in shared mode, and
do the logging.

- Melanie