Re: Should vacuum process config file reload more often

On Tue, Mar 28, 2023 at 4:21 AM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)com> wrote:
>
> At Mon, 27 Mar 2023 14:12:03 -0400, Melanie Plageman <melanieplageman(at)gmail(dot)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.

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

> 0004:
> - double at_vacuum_cost_delay;
> - int at_vacuum_cost_limit;
> + double at_table_option_vac_cost_delay;
> + int at_table_option_vac_cost_limit;
>
> We call that options "relopt(ion)". I don't think there's any reason
> to use different names.

I've updated the names.

> dlist_head av_runningWorkers;
> WorkerInfo av_startingWorker;
> AutoVacuumWorkItem av_workItems[NUM_WORKITEMS];
> + pg_atomic_uint32 av_nworkers_for_balance;
>
> The name of the new member doesn't seem to follow the surrounding
> convention. (However, I don't think the member is needed. See below.)

I've updated the name to fit the convention better.

> - /*
> - * Remember the prevailing values of the vacuum cost GUCs. We have to
> - * restore these at the bottom of the loop, else we'll compute wrong
> - * values in the next iteration of autovac_balance_cost().
> - */
> - stdVacuumCostDelay = VacuumCostDelay;
> - stdVacuumCostLimit = VacuumCostLimit;
> + av_table_option_cost_limit = tab->at_table_option_vac_cost_limit;
> + av_table_option_cost_delay = tab->at_table_option_vac_cost_delay;
>
> I think this requires a comment.

I've added one.

>
> + /* 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
be balanced across all of the workers currently actively vacuuming
tables without cost-related table options.

There shouldn't be a way for this to be zero, since this worker calls
autovac_balance_cost() before it starts vacuuming the table. I wanted to
rule out any possibility of a divide by 0 issue. I have changed it to an
assert instead.

> +/*
> + * autovac_balance_cost
> + * Recalculate the number of workers to consider, given table options and
> + * the current number of active workers.
> + *
> + * Caller must hold the AutovacuumLock in at least shared mode.
>
> The function name doesn't seem align with what it does. However, I
> mentioned above that it might be unnecessary.

This is the same name as the function had previously. However, I think
it does make sense to rename it. The cost limit must be balanced across
the workers. This function calculated how many workers the cost limit
should be balanced across. I renamed it to
autovac_recalculate_workers_for_balance()

> +AutoVacuumUpdateLimit(void)
>
> 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.

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

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.

- Melanie