Re: Should vacuum process config file reload more often

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.

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

0002:

I felt a bit uneasy on this. It seems somewhat complex (and makes the
succeeding patches complex), has confusing names, and doesn't seem
like self-contained. 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.

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.

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.

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

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

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

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

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

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

regars.

--
Kyotaro Horiguchi
NTT Open Source Software Center