Re: Should vacuum process config file reload more often

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.mshk@gmail.com> wrote:
>
> On Wed, Apr 5, 2023 at 5:05 AM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> >
> > On Tue, Apr 4, 2023 at 4:27 AM Masahiko Sawada <sawada.mshk@gmail.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()...

On Wed, Apr 5, 2023 at 3:16 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> About 0001:
>
> + * VacuumFailsafeActive is a defined as a global so that we can determine
> + * whether or not to re-enable cost-based vacuum delay when vacuuming a table.
> + * If failsafe mode has been engaged, we will not re-enable cost-based delay
> + * for the table until after vacuuming has completed, regardless of other
> + * settings. Only VACUUM code should inspect this variable and only table
> + * access methods should set it. In Table AM-agnostic VACUUM code, this
> + * variable controls whether or not to allow cost-based delays. Table AMs are
> + * free to use it if they desire this behavior.
> + */
> +bool           VacuumFailsafeActive = false;
>
> If I understand this correctly, there seems to be an issue. The
> AM-agnostic VACUUM code is setting it and no table AMs actually do
> that.

No, it is not set in table AM-agnostic VACUUM code. I meant it is
used/read from/inspected in table AM-agnostic VACUUM code. Table AMs can
set it if they want to avoid cost-based delays being re-enabled. It is
only set to true heap-specific code and is initialized to false and
reset in table AM-agnostic code back to false in between each relation
being vacuumed. I updated the comment to reflect this. Let me know if
you think it is clear.

> 0003:
> +
> +                       /*
> +                        * Ensure VacuumFailsafeActive has been reset before vacuuming the
> +                        * next relation.
> +                        */
> +                       VacuumFailsafeActive = false;
>                 }
>         }
>         PG_FINALLY();
>         {
>                 in_vacuum = false;
>                 VacuumCostActive = false;
> +               VacuumFailsafeActive = false;
> +               VacuumCostBalance = 0;
>
> There is no need to reset VacuumFailsafeActive in the PG_TRY() block.

I think that is true -- since it is initialized to false and reset to
false after vacuuming every relation. However, I am leaning toward
keeping it because I haven't thought through every codepath and
determined if there is ever a way where it could be true here.

> +       /*
> +        * 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 && IsAutoVacuumWorkerProcess())
> +       {
> +               ConfigReloadPending = false;
> +               ProcessConfigFile(PGC_SIGHUP);
> +               VacuumUpdateCosts();
> +       }
>
> I believe we should prevent unnecessary reloading when
> VacuumFailsafeActive is true.

This is in conflict with two of the other reviewers feedback:
Sawada-san:

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

and Daniel in response to this:

> > 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.
>
> I agree with this.

> +               AutoVacuumUpdateLimit();
>
> I'm not entirely sure, but it might be better to name this
> AutoVacuumUpdateCostLimit().

I have made this change.

> +       pg_atomic_flag wi_dobalance;
> ...
> +               /*
> +                * We only expect this worker to ever set the flag, so don't bother
> +                * checking the return value. We shouldn't have to retry.
> +                */
> +               if (tab->at_dobalance)
> +                       pg_atomic_test_set_flag(&MyWorkerInfo->wi_dobalance);
> +               else
> +                       pg_atomic_clear_flag(&MyWorkerInfo->wi_dobalance);
>
>                 LWLockAcquire(AutovacuumLock, LW_SHARED);
>
>                 autovac_recalculate_workers_for_balance();
>
> I don't see the need for using atomic here. The code is executed
> infrequently and we already take a lock while counting do_balance
> workers. So sticking with the old locking method (taking LW_EXCLUSIVE
> then set wi_dobalance then do balance) should be fine.

We access wi_dobalance on every call to AutoVacuumUpdateLimit() which is
executed in vacuum_delay_point(). I do not think we can justify take a
shared lock in a function that is called so frequently.

> +void
> +AutoVacuumUpdateLimit(void)
> ...
> +       if (av_relopt_cost_limit > 0)
> +               VacuumCostLimit = av_relopt_cost_limit;
> +       else
>
> I think we should use wi_dobalance to decide if we need to do balance
> or not. We don't need to take a lock to do that since only the process
> updates it.

We do do that below in the "else" before balancing. But we for sure
don't need to balance if relopt for cost limit is set. We can save an
access to an atomic variable this way. I think the atomic is a
relatively cheap way of avoiding this whole locking question.

> /*
>                  * Remove my info from shared memory.  We could, but intentionally
> -                * don't, clear wi_cost_limit and friends --- this is on the
> -                * assumption that we probably have more to do with similar cost
> -                * settings, so we don't want to give up our share of I/O for a very
> -                * short interval and thereby thrash the global balance.
> +                * don't, unset wi_dobalance on the assumption that we are more likely
> +                * than not to vacuum a table with no table options next, so we don't
> +                * want to give up our share of I/O for a very short interval and
> +                * thereby thrash the global balance.
>                  */
>                 LWLockAcquire(AutovacuumScheduleLock, LW_EXCLUSIVE);
>                 MyWorkerInfo->wi_tableoid = InvalidOid;
>
> The comment mentions wi_dobalance, but it doesn't appear here..

The point of the comment is that we don't do anything with wi_dobalance
here. It is explaining why it doesn't appear. The previous comment
mentioned not doing anything with wi_cost_delay and wi_cost_limit which
also didn't appear here.

On Wed, Apr 5, 2023 at 9:10 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 4 Apr 2023, at 22:04, Melanie Plageman <melanieplageman@gmail.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.

I totally see what you are saying. The only complication is that all of
the other variables used in vacuum code are the camelcase and the gucs
follow the snake case -- as pointed out in a previous review comment by
Sawada-san:

> @@ -83,6 +84,7 @@ int                   vacuum_cost_limit;
>   */
>  int                    VacuumCostLimit = 0;
>  double         VacuumCostDelay = -1;
> +static bool vacuum_can_reload_config = false;
>
> In vacuum.c, we use snake case for GUC parameters and camel case for
> other global variables, so it seems better to rename it
> VacuumCanReloadConfig. Sorry, that's my fault.

This is less of a compelling argument than subtle breakage for extension
code, though.

I am, however, wondering if extensions expect to have access to the guc
variable or the global variable -- or both?

Left it as is in this version until we resolve the question.

> > Oh, also I've annotated these with PGDLLIMPORT too.
> >
> >> + * TODO: should VacuumCostLimit and VacuumCostDelay be initialized to valid or
> >> + * invalid values?
> >> + */
> >> +int                    VacuumCostLimit = 0;
> >> +double         VacuumCostDelay = -1;
> >>
> >> I think the important part is to make sure they are never accessed without
> >> VacuumUpdateCosts having been called first.  I think that's the case here, but
> >> it's not entirely clear.  Do you see a codepath where that could happen?  If
> >> they are initialized to a sentinel value we also need to check for that, so
> >> initializing to the defaults from the corresponding GUCs seems better.
> >
> > I don't see a case where autovacuum could access these without calling
> > VacuumUpdateCosts() first. I think the other callers of
> > vacuum_delay_point() are the issue (gist/gin/hash/etc).
> >
> > It might need a bit more thought.
> >
> > My concern was that these variables correspond to multiple GUCs each
> > depending on the backend type, and those backends have different
> > defaults (e.g. autovac workers default cost delay is different than
> > client backend doing vacuum cost delay).
> >
> > However, what I have done in this version is initialize them to the
> > defaults for a client backend executing VACUUM or ANALYZE, since I am
> > fairly confident that autovacuum will not use them without calling
> > VacuumUpdateCosts().
>
> Another question along these lines, we only call AutoVacuumUpdateLimit() in
> case there is a sleep in vacuum_delay_point():
>
> +       /*
> +        * 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.
> +        */
> +       AutoVacuumUpdateLimit();
>
> Shouldn't we always call that in case we had a config reload, or am I being
> thick?

We actually also call it from inside VacuumUpdateCosts(), which is
always called in the case of a config reload.

> >> +static double av_relopt_cost_delay = -1;
> >> +static int av_relopt_cost_limit = 0;
>
> Sorry, I didn't catch this earlier, shouldn't this be -1 to match the default
> value of autovacuum_vacuum_cost_limit?

Yea, this is a bit tricky. Initial values of -1 and 0 have the same
effect when we are referencing av_relopt_vacuum_cost_limit in
AutoVacuumUpdateCostLimit(). However, I was trying to initialize both
av_relopt_vacuum_cost_limit and av_relopt_vacuum_cost_delay to "invalid"
values which were not the default for the associated autovacuum gucs,
since initializing av_relopt_cost_delay to the default for
autovacuum_vacuum_cost_delay (2 ms) would cause it to be used even if
storage params were not set for the relation.

I have updated the initial value to -1, as you suggested -- but I don't
know if it is more or less confusing the explain what I just explained
in the comment above it.

> >> These need a comment IMO, ideally one that explain why they are initialized to
> >> those values.
> >
> > I've added a comment.
>
> + * Variables to save the cost-related table options for the current relation
>
> The "table options" nomenclature is right now only used for FDW foreign table
> options, I think we should use "storage parameters" or "relation options" here.

I've updated these to "storage parameters" to match the docs. I poked
around looking for other places I referred to them as table options and
tried to fix those as well. I've also changed all relevant variable
names.

> >> +       /* There is at least 1 autovac worker (this worker). */
> >> +       Assert(nworkers_for_balance > 0);
> >>
> >> Is there a scenario where this is expected to fail?  If so I think this should
> >> be handled and not just an Assert.
> >
> > No, this isn't expected to happen because an autovacuum worker would
> > have called autovac_recalculate_workers_for_balance() before calling
> > VacuumUpdateCosts() (which calls AutoVacuumUpdateLimit()) in
> > do_autovacuum(). But, if someone were to move around or add a call to
> > VacuumUpdateCosts() there is a chance it could happen.
>
> Thinking more on this I'm tempted to recommend that we promote this to an
> elog(), mainly due to the latter.  An accidental call to VacuumUpdateCosts()
> doesn't seem entirely unlikely to happen

Makes sense. I've added a trivial elog ERROR, but I didn't spend quite
enough time thinking about what (if any) other context to include in it.

- Melanie