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

Thanks for the detailed review!

On Tue, Mar 28, 2023 at 11:09 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Tue, 28 Mar 2023 20:35:28 -0400, Melanie Plageman <melanieplageman@gmail.com> wrote in
> > On Tue, Mar 28, 2023 at 4:21 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
> > >
> > > At Mon, 27 Mar 2023 14:12:03 -0400, Melanie Plageman <melanieplageman@gmail.com> wrote in
> > >
> > > 0002:
> > >
> > > I felt a bit uneasy on this. It seems somewhat complex (and makes the
> > > succeeding patches complex),
> >
> > Even if we introduced a second global variable to indicate that failsafe
> > mode has been engaged, we would still require the additional checks
> > of VacuumCostInactive.
> >
> > > has confusing names,
> >
> > I would be happy to rename the values of the enum to make them less
> > confusing. Are you thinking "force" instead of "locked"?
> > maybe:
> > VACUUM_COST_FORCE_INACTIVE and
> > VACUUM_COST_INACTIVE
> > ?
> >
> > > and doesn't seem like self-contained.
> >
> > By changing the variable from VacuumCostActive to VacuumCostInactive, I
> > have kept all non-vacuum code from having to distinguish between it
> > being inactive due to failsafe mode or due to user settings.
>
> My concern is that VacuumCostActive is logic-inverted and turned into
> a ternary variable in a subtle way. The expression
> "!VacuumCostInactive" is quite confusing. (I sometimes feel the same
> way about "!XLogRecPtrIsInvalid(lsn)", and I believe most people write
> it with another macro like "lsn != InvalidXLogrecPtr"). Additionally,
> the constraint in this patch will be implemented as open code.  So I
> wanted to suggest something like the attached. The main idea is to use
> a wrapper function to enforce the restriction, and by doing so, we
> eliminated the need to make the variable into a ternary without a good
> reason.

So, the rationale for making it a ternary is that the variable is the
combination of two pieces of information which has only has 3 valid
states:
failsafe inactive + cost active = cost active
failsafe inactive + cost inactive = cost inactive
failsafe active + cost inactive = cost inactive and locked
the fourth is invalid
failsafe active + cost active = invalid
That is harder to enforce with two variables.
Also, the two pieces of information are not meaningful individually.
So, I thought it made sense to make a single variable.

Your suggested patch introduces an additional variable which shadows
LVRelState->failsafe_active but doesn't actually get set/reset at all of
the correct places. If we did introduce a second global variable, I
don't think we should also keep LVRelState->failsafe_active, as keeping
them in sync will be difficult.

As for the double negative (!VacuumCostInactive), I agree that it is not
ideal, however, if we use a ternary and keep VacuumCostActive, there is
no way for non-vacuum code to treat it as a boolean.
With the ternary VacuumCostInactive, only vacuum code has to know about
the distinction between inactive+failsafe active and inactive+failsafe
inactive.

As for the setter function, I think that having a function to set
VacuumCostActive based on failsafe_active is actually doing more harm
than good. Only vacuum code has to know about the distinction as it is,
so we aren't really saving any trouble (there would really only be two
callers of the suggested function). And, since the function hides
whether or not VacuumCostActive was actually set to the passed-in value,
we can't easily do other necessary maintenance -- like zero out
VacuumCostBalance if we disabled vacuum cost.

> > > 0003:
> > >
> > > +        * Reload the configuration file if requested. This allows changes to
> > > +        * vacuum_cost_limit and vacuum_cost_delay to take effect while a table is
> > > +        * being vacuumed or analyzed. Analyze should not reload configuration
> > > +        * file if it is in an outer transaction, as GUC values shouldn't be
> > > +        * allowed to refer to some uncommitted state (e.g. database objects
> > > +        * created in this transaction).
> > >
> > > I'm not sure GUC reload is or should be related to transactions. For
> > > instance, work_mem can be changed by a reload during a transaction
> > > unless it has been set in the current transaction. I don't think we
> > > need to deliberately suppress changes in variables caused by realods
> > > during transactions only for analzye. If analyze doesn't like changes
> > > to certain GUC variables, their values should be snapshotted before
> > > starting the process.
> >
> > Currently, we only reload the config file in top-level statements. We
> > don't reload the configuration file from within a nested transaction
> > command. BEGIN starts a transaction but not a transaction command. So
> > BEGIN; ANALYZE; probably wouldn't violate this rule. But it is simpler
> > to just forbid reloading when it is not a top-level transaction command.
> > I have updated the comment to reflect this.
>
> I feel it's a bit fragile. We may not be able to manage the reload
> timeing perfectly. I think we might accidentally add a reload
> timing. In that case, the assumption could break. In most cases, I
> think we use snapshotting in various ways to avoid unintended variable
> changes. (And I beilieve the analyze code also does that.)

I'm not sure I fully understand the problem you are thinking of. What do
you mean about managing the reload timing? Are you suggesting there is a
problem with excluding analzye in an outer transaction from doing the
reload or with doing the reload during vacuum and analyze when they are
top-level statements?

And, by snapshotting do you mean how vacuum_rel() and do_analyze_rel() do
NewGUCNestLevel() so that they can then do AtEOXact_GUC() and rollback
guc changes done during that operation?
How are you envisioning that being used here?

On Wed, Mar 29, 2023 at 2:00 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Wed, 29 Mar 2023 13:21:55 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
> > autovacuum.c:2893
> >               /*
> >                * If any of the cost delay parameters has been set individually for
> >                * this table, disable the balancing algorithm.
> >                */
> >               tab->at_dobalance =
> >                       !(avopts && (avopts->vacuum_cost_limit > 0 ||
> >                                                avopts->vacuum_cost_delay > 0));
> >
> > So, sorry for the noise. I'll review it while this into cnosideration.
>
> Then I found that the code is quite confusing as it is.
>
> For the tables that don't have cost_delay and cost_limit specified
> indificually, at_vacuum_cost_limit and _delay store the system global
> values detemined by GUCs. wi_cost_delay, _limit and _limit_base stores
> the same values with them. As the result I concluded tha
> autovac_balance_cost() does exactly what Melanie's patch does, except
> that nworkers_for_balance is not stored in shared memory.
>
> I discovered that commit 1021bd6a89 brought in do_balance.
>
> >    Since the mechanism is already complicated, just disable it for those
> >    cases rather than trying to make it cope.  There are undesirable
>
> After reading this, I get why the code is so complex.  It is a remnant
> of when balancing was done with tables that had individually specified
> cost parameters. And I found the following description in the doc.
>
> https://www.postgresql.org/docs/devel/routine-vacuuming.html
> > When multiple workers are running, the autovacuum cost delay
> > parameters (see Section 20.4.4) are “balanced” among all the running
> > workers, so that the total I/O impact on the system is the same
> > regardless of the number of workers actually running. However, any
> > workers processing tables whose per-table
> > autovacuum_vacuum_cost_delay or autovacuum_vacuum_cost_limit storage
> > parameters have been set are not considered in the balancing
> > algorithm.
>
> The initial balancing mechanism was brought in by e2a186b03c back in
> 2007. The balancing code has had that unnecessarily complexity ever
> since.
>
> Since I can't think of a better idea than Melanie's proposal for
> handling this code, I'll keep reviewing it with that approach in mind.

Thanks for doing this archaeology. I didn't know the history of dobalance
and hadn't looked into 1021bd6a89.
I was a bit confused by why dobalance was false even if only table
option cost delay is set and not table option cost limit.

I think we can retain this behavior for now, but it may be worth
re-examining in the future.

On Wed, Mar 29, 2023 at 4:35 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Wed, 29 Mar 2023 13:21:55 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
> > So, sorry for the noise. I'll review it while this into cnosideration.
>
> 0003:
>
> It's not this patche's fault, but I don't like the fact that the
> variables used for GUC, VacuumCostDelay and VacuumCostLimit, are
> updated outside the GUC mechanism. Also I don't like the incorrect
> sorting of variables, where some working variables are referred to as
> GUC parameters or vise versa.
>
> Although it's somewhat unrelated to the goal of this patch, I think we
> should clean up the code tidy before proceeding. Shouldn't we separate
> the actual parameters from the GUC base variables, and sort out the
> all related variaghble? (something like the attached, on top of your
> patch.)

So, I agree we should separate the parameters used in the code from the
GUC variables -- since there are multiple users with different needs
(autovac workers, parallel vac workers, and vacuum). However, I was
hesitant to tackle that here.

I'm not sure how these changes will impact extensions that rely on
these vacuum parameters and their direct relationship to the guc values.

In your patch, you didn't update the parameter with the guc value of
vacuum_cost_limit and vacuum_cost_delay, but were we to do so, we would
need to make sure it was updated every time after a config reload. This
isn't hard to do in the current code, but I'm not sure how we can ensure
that future callers of ProcessConfigFile() in vacuum code always update
these values afterward. Perhaps we could add some after_reload hook?
Which does seem like a larger project.

> I have some comments on 0003 as-is.
>
> +               tab->at_relopt_vac_cost_limit = avopts ?
> +                       avopts->vacuum_cost_limit : 0;
> +               tab->at_relopt_vac_cost_delay = avopts ?
> +                       avopts->vacuum_cost_delay : -1;
>
> The value is not used when do_balance is false, so I don't see a
> specific reason for these variables to be different when avopts is
> null.

Actually we need to set these to 0 and -1, because we set
av_relopt_cost_limit and av_relopt_cost_delay with them and those values
are checked regardless of wi_dobalance.

We need to do this because we want to use the correct value to override
VacuumCostLimit and VacuumCostDelay. wi_dobalance may be false because
we have a table option cost delay but we have no table option cost
limit. When we override VacuumCostDelay, we want to use the table option
value but when we override VacuumCostLimit, we want to use the regular
value. We need these initialized to values that will allow us to do
that.

> +autovac_recalculate_workers_for_balance(void)
> +{
> +       dlist_iter      iter;
> +       int                     orig_nworkers_for_balance;
> +       int                     nworkers_for_balance = 0;
> +
> +       if (autovacuum_vac_cost_delay == 0 ||
> +               (autovacuum_vac_cost_delay == -1 && VacuumCostDelay == 0))
>                 return;
> +       if (autovacuum_vac_cost_limit <= 0 && VacuumCostLimit <= 0)
> +               return;
> +
>
> I'm not quite sure how these conditions relate to the need to count
> workers that shares the global I/O cost.

Ah, this is a good point, we should still keep this number up-to-date
even if the costs are disabled at the time we are checking it in case
cost-based delays are re-enabled later before we recalculate this
number.  I had this code originally because autovac_balance_cost() would
exit early if cost-based delays were disabled -- but this only worked
because they couldn't be re-enabled during vacuuming a table and
autovac_balance_cost() was called always in between vacuuming tables.

I've removed these lines.

And perhaps there is an argument for calling
autovac_recalculate_workers_for_balance() in vacuum_delay_point() after
reloading the config file...
I have not done so in attached version.

> (Though I still believe this funtion might not be necessary.)

I don't see how we can do without this function. We need an up-to-date
count of the number of autovacuum workers vacuuming tables which do not
have vacuum cost-related table options.


> +       if (av_relopt_cost_limit > 0)
> +               VacuumCostLimit = av_relopt_cost_limit;
> +       else
> +       {
> +               av_base_cost_limit = autovacuum_vac_cost_limit > 0 ?
> +                       autovacuum_vac_cost_limit : VacuumCostLimit;
> +
> +               AutoVacuumBalanceLimit();
>
> I think each worker should use MyWorkerInfo->wi_dobalance to identyify
> whether the worker needs to use balanced cost values.

Ah, there is a bug here. I have fixed it by making wi_dobalance an
atomic flag so that we can check it before calling
AutoVacuumBalanceLimit() (without taking a lock).

I don't see other (non-test code) callers using atomic flags, so I can't
tell if we need to loop to ensure that pg_atomic_test_set_flag() returns
true.

> +void
> +AutoVacuumBalanceLimit(void)
>
> I'm not sure this function needs to be a separate function.

We need to call it more often than we can call AutoVacuumUpdateLimit(),
so the logic needs to be separate. Are you suggesting we inline the
logic in the two places it is needed?

v11 attached with updates mentioned above.

- Melanie