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

On Tue, Apr 4, 2023 at 4:27 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> The 0001 patch mostly looks good to me except for one
> point:
>
> @@ -391,7 +389,7 @@ heap_vacuum_rel(Relation rel, VacuumParams *params,
>          Assert(params->index_cleanup != VACOPTVALUE_UNSPECIFIED);
>          Assert(params->truncate != VACOPTVALUE_UNSPECIFIED &&
>                     params->truncate != VACOPTVALUE_AUTO);
> -        vacrel->failsafe_active = false;
> +        VacuumFailsafeActive = false;
>          vacrel->consider_bypass_optimization = true;
>          vacrel->do_index_vacuuming = true;
>
> Looking at the 0003 patch, we set VacuumFailsafeActive to false per table:
>
> +                        /*
> +                         * Ensure VacuumFailsafeActive has been reset
> before vacuuming the
> +                         * next relation relation.
> +                         */
> +                        VacuumFailsafeActive = false;
>
> Given that we ensure it's reset before vacuuming the next table, do we
> need to reset it in heap_vacuum_rel?

I've changed the one in heap_vacuum_rel() to an assert.

> (there is a typo; s/relation relation/relation/)

Thanks! fixed.

> > > 0002:
> > >
> > > @@ -2388,6 +2398,7 @@ vac_max_items_to_alloc_size(int max_items)
> > >          return offsetof(VacDeadItems, items) +
> > > sizeof(ItemPointerData) * max_items;
> > >  }
> > >
> > > +
> > >  /*
> > >   *     vac_tid_reaped() -- is a particular tid deletable?
> > >   *
> > >
> > > Unnecessary new line. There are some other unnecessary new lines in this patch.
> >
> > Thanks! I think I got them all.
> >
> > > ---
> > > @@ -307,6 +309,8 @@ extern PGDLLIMPORT pg_atomic_uint32 *VacuumActiveNWorkers;
> > >  extern PGDLLIMPORT int VacuumCostBalanceLocal;
> > >
> > >  extern bool VacuumFailsafeActive;
> > > +extern int     VacuumCostLimit;
> > > +extern double VacuumCostDelay;
> > >
> > > and
> > >
> > > @@ -266,8 +266,6 @@ extern PGDLLIMPORT int max_parallel_maintenance_workers;
> > >  extern PGDLLIMPORT int VacuumCostPageHit;
> > >  extern PGDLLIMPORT int VacuumCostPageMiss;
> > >  extern PGDLLIMPORT int VacuumCostPageDirty;
> > > -extern PGDLLIMPORT int VacuumCostLimit;
> > > -extern PGDLLIMPORT double VacuumCostDelay;
> > >
> > > Do we need PGDLLIMPORT too?
> >
> > I was on the fence about this. I annotated the new guc variables
> > vacuum_cost_delay and vacuum_cost_limit with PGDLLIMPORT, but I did not
> > annotate the variables used in vacuum code (VacuumCostLimit/Delay). I
> > think whether or not this is the right choice depends on two things:
> > whether or not my understanding of PGDLLIMPORT is correct and, if it is,
> > whether or not we want extensions to be able to access
> > VacuumCostLimit/Delay or if just access to the guc variables is
> > sufficient/desirable.
>
> I guess it would be better to keep both accessible for backward
> compatibility. Extensions are able to access both GUC values and
> values that are actually used for vacuum delays (as we used to use the
> same variables).

> Here are some review comments for 0002-0004 patches:
>
> 0002:
> -        if (MyWorkerInfo)
> +        if (am_autovacuum_launcher)
> +                return;
> +
> +        if (am_autovacuum_worker)
>          {
> -                VacuumCostDelay = MyWorkerInfo->wi_cost_delay;
>                  VacuumCostLimit = MyWorkerInfo->wi_cost_limit;
> +                VacuumCostDelay = MyWorkerInfo->wi_cost_delay;
> +        }
>
> Isn't it a bit safer to check MyWorkerInfo instead of
> am_autovacuum_worker?

Ah, since we access it. I've made the change.

> Also, I don't think there is any reason why we want to exclude only
> the autovacuum launcher.

My rationale is that the launcher is the only other process type which
might reasonably be executing this code besides autovac workers, client
backends doing VACUUM/ANALYZE, and parallel vacuum workers. Is it
confusing to have the launcher have VacuumCostLimt and VacuumCostDelay
set to the guc values for explicit VACUUM and ANALYZE -- even if the
launcher doesn't use these variables?

I've removed the check, because I do agree with you that it may be
unnecessarily confusing in the code.

> ---
> + * TODO: should VacuumCostLimit and VacuumCostDelay be initialized to valid or
> + * invalid values?
>
> How about using the default value of normal backends, 200 and 0?

I've gone with this suggestion

> 0003:
>
> @@ -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.

I have renamed it.

> 0004:
>
> +                if (tab->at_dobalance)
> +                        pg_atomic_test_set_flag(&MyWorkerInfo->wi_dobalance);
> +                else
>
> The comment of pg_atomic_test_set_flag() says that it returns false if
> the flag has not successfully been set:
>
>  * pg_atomic_test_set_flag - TAS()
>  *
>  * Returns true if the flag has successfully been set, false otherwise.
>  *
>  * Acquire (including read barrier) semantics.
>
> But IIUC we don't need to worry about that as only one process updates
> the flag, right? It might be a good idea to add some comments why we
> don't need to check the return value.

I have added this comment.

> ---
> -                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()?


On Tue, Apr 4, 2023 at 9:36 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>

Thanks for the review!

> > On 4 Apr 2023, at 00:35, Melanie Plageman <melanieplageman@gmail.com> wrote:
> >
> > On Mon, Apr 3, 2023 at 3:08 PM Andres Freund <andres@anarazel.de> wrote:
> >> On 2023-04-03 14:43:14 -0400, Tom Lane wrote:
> >>> Melanie Plageman <melanieplageman@gmail.com> writes:
> >>>> v13 attached with requested updates.
> >>>
> >>> I'm afraid I'd not been paying any attention to this discussion,
> >>> but better late than never.  I'm okay with letting autovacuum
> >>> processes reload config files more often than now.  However,
> >>> I object to allowing ProcessConfigFile to be called from within
> >>> commands in a normal user backend.  The existing semantics are
> >>> that user backends respond to SIGHUP only at the start of processing
> >>> a user command, and I'm uncomfortable with suddenly deciding that
> >>> that can work differently if the command happens to be VACUUM.
> >>> It seems unprincipled and perhaps actively unsafe.
> >>
> >> I think it should be ok in commands like VACUUM that already internally start
> >> their own transactions, and thus require to be run outside of a transaction
> >> and at the toplevel. I share your concerns about allowing config reload in
> >> arbitrary places. While we might want to go there, it would require a lot more
> >> analysis.
>
> Thinking more on this I'm leaning towards going with allowing more frequent
> reloads in autovacuum, and saving the same for VACUUM for more careful study.
> The general case is probably fine but I'm not convinced that there aren't error
> cases which can present unpleasant scenarios.

In attached v15, I've dropped support for VACUUM and non-nested ANALYZE.
It is like a 5 line change and could be added back at any time.

> > As an alternative for your consideration, attached v14 set implements
> > the config file reload for autovacuum only (in 0003) and then enables it
> > for VACUUM and ANALYZE not in a nested transaction command (in 0004).
> >
> > Previously I had the commits in the reverse order for ease of review (to
> > separate changes to worker limit balancing logic from config reload
> > code).
>
> A few comments on top of already submitted reviews, will do another pass over
> this later today.
>
> + * 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.
>
> This comment should be expanded to document who we expect to inspect this
> variable in order to decide on cost-based vacuum.
>
> Moving the failsafe switch into a global context means we face the risk of an
> extension changing it independently of the GUCs that control it (or the code
> relying on it) such that these are out of sync.  External code messing up
> internal state is not new and of course outside of our control, but it's worth
> at least considering.  There isn't too much we can do here, but perhaps expand
> this comment to include a "do not change this" note?

I've updated the comment to mention how table AM-agnostic VACUUM code
uses it and to say that table AMs can set it if they want that behavior.

> +extern bool VacuumFailsafeActive;
>
> While I agree with upthread review comments that extensions shoulnd't poke at
> this, not decorating it with PGDLLEXPORT adds little protection and only cause
> inconsistencies in symbol exports across platforms.  We only explicitly hide
> symbols in shared libraries IIRC.

I've updated this.

> +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?

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

>
> +* Update VacuumCostLimit with the correct value for an autovacuum worker, given
>
> Trivial whitespace error in function comment.

Fixed.

> +static double av_relopt_cost_delay = -1;
> +static int av_relopt_cost_limit = 0;
>
> These need a comment IMO, ideally one that explain why they are initialized to
> those values.

I've added a comment.

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

- Melanie