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

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
> > > So, I've attached an alternate version of the patchset which takes the
> > > approach of having one commit which only enables cost-based delay GUC
> > > refresh for VACUUM and another commit which enables it for autovacuum
> > > and makes the changes to balancing variables.
> > >
> > > I still think the commit which has workers updating their own
> > > wi_cost_delay in vacuum_delay_point() is a bit weird. It relies on no one
> > > else emulating our bad behavior and reading from wi_cost_delay without a
> > > lock and on no one else deciding to ever write to wi_cost_delay (even
> > > though it is in shared memory [this is the same as master]). It is only
> > > safe because our process is the only one (right now) writing to
> > > wi_cost_delay, so when we read from it without a lock, we know it isn't
> > > being written to. And everyone else takes a lock when reading from
> > > wi_cost_delay right now. So, it seems...not great.
> > >
> > > This approach also introduces a function that is only around for
> > > one commit until the next commit obsoletes it, which seems a bit silly.
> >
> > (I'm not sure what this refers to, though..) I don't think it's silly
> > if a later patch removes something that the preceding patches
> > introdcued, as long as that contributes to readability.  Untimately,
> > they will be merged together on committing.
> 
> I was under the impression that reviewers thought config reload and
> worker balance changes should be committed in separate commits.
> 
> Either way, the ephemeral function is not my primary concern. I felt
> more uncomfortable with increasing how often we update a double in
> shared memory which is read without acquiring a lock.
> 
> > > Basically, I think it is probably better to just have one commit
> > > enabling guc refresh for VACUUM and then another which correctly
> > > implements what is needed for autovacuum to do the same.
> > > Attached v9 does this.
> > >
> > > I've provided both complete versions of both approaches (v9 and v8).
> >
> > I took a look at v9 and have a few comments.
> >
> > 0001:
> >
> > I don't believe it is necessary, as mentioned in the commit
> > message. It apperas that we are resetting it at the appropriate times.
> 
> VacuumCostBalance must be zeroed out when we disable vacuum cost.
> Previously, we did not reenable VacuumCostActive once it was disabled,
> but now that we do, I think it is good practice to always zero out
> VacuumCostBalance when we disable vacuum cost. I will squash this commit
> into the one introducing VacuumCostInactive, though.

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

> > I think it'd be simpler to add a global boolean (maybe
> > VacuumCostActiveForceDisable or such) that forces VacuumCostActive to
> > be false and set VacuumCostActive using a setter function that follows
> > the boolean.
> 
> I think maintaining an additional global variable is more brittle than
> including the information in a single variable.
> 
> > 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.)

> > +               /* There is at least 1 autovac worker (this worker). */
> > +               int                     nworkers_for_balance = Max(pg_atomic_read_u32(
> > +                                                               &AutoVacuumShmem->av_nworkers_for_balance), 1);
> >
> > I think it *must* be greater than 0. However, to begin with, I don't
> > think we need that variable to be shared. I don't believe it matters
> > if we count involved workers every time we calculate the delay.
> 
> We are not calculating the delay but the cost limit. The cost limit must

Ah, right, it's limit, but my main point still stands.

> be balanced across all of the workers currently actively vacuuming
> tables without cost-related table options.

The purpose of the old autovac_balance_cost() is to distribute the
cost among all involved tables, proportionally based on each worker's
cost specification.  Adjusting the limit just for tables affected by
reloads disrupts the cost balance.

> > If I'm not missing anything, this function does something quite
> > different from the original autovac_balance_cost().  The original
> > function distributes the total cost based on the GUC variables among
> > workers proportionally according to each worker's cost
> > parameters. Howevwer, this function distributes the total cost
> > equally.
> 
> Yes, as I mentioned in the commit message, because all the workers now
> have no reason to have different cost parameters (due to reloading the
> config file on almost every page), there is no reason to use ratios.
> Workers vacuuming a table with no cost-related table options simply need
> to divide the limit equally amongst themselves because they all will
> have the same limit and delay values.

I'm not sure about the assumption in the commit message. For instance,
if the total cost limit drops significantly, it's possible that the
workers left out of this calculation might end up using all the
reduced cost. Wouldn't this imply that all workers should recompute
their individual limits?

> >
> > +               int                     vac_cost_limit = autovacuum_vac_cost_limit > 0 ?
> > +               autovacuum_vac_cost_limit : VacuumCostLimit;
> > ...
> > +               int                     balanced_cost_limit = vac_cost_limit / nworkers_for_balance;
> > ...
> > +               VacuumCostLimit = Max(Min(balanced_cost_limit, vac_cost_limit), 1);
> >         }
> >
> > This seems to repeatedly divide VacuumCostLimit by
> > nworkers_for_balance. I'm not sure, but this function might only be
> > called after a reload. If that's the case, I don't think it's safe
> > coding, even if it works.
> 
> Good point about repeatedly dividing VacuumCostLimit by
> nworkers_for_balance. I've added a variable to keep track of the base
> cost limit and separated the functionality of updating the limit into
> two parts -- one AutoVacuumUpdateLimit() which is only meant to be
> called after reload and references VacuumCostLimit to set the
> av_base_cost_limit and another, AutoVacuumBalanceLimit(), which only
> overrides VacuumCostLimit but uses av_base_cost_limit.

Sorry, but will check this later.

> I've noted in the comments that AutoVacuumBalanceLimit() should be
> called to adjust to a potential change in nworkers_for_balance
> (currently every time after we sleep in vacuum_delay_point()) and
> AutoVacuumUpdateLimit() should only be called once after a config
> reload, as it references VacuumCostLimit.
> 
> I will note that this problem also exists in master, as
> autovac_balance_cost references VacuumCostLimit in order to set worker
> cost limits and then AutoVacuumUpdateDelay() overrides VacuumCostLimit
> with the value calculated in autovac_balance_cost() from
> VacuumCostLimit.
> 
> v10 attached with mentioned updates.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center