Re: Should vacuum process config file reload more often

> On 23 Mar 2023, at 07:08, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> On Sun, Mar 19, 2023 at 7:47 AM Melanie Plageman <melanieplageman(at)gmail(dot)com> wrote:

> It makes sense to me that we need to reload the config file even when
> vacuum-delay is disabled. But I think it's not convenient for users
> that we don't reload the configuration file once the failsafe is
> triggered. I think users might want to change some GUCs such as
> log_autovacuum_min_duration.

I agree with this.

>> On an unrelated note, I was wondering if there were any docs anywhere
>> that should be updated to go along with this.
>
> The current patch improves the internal mechanism of (re)balancing
> vacuum-cost but doesn't change user-visible behavior. I don't have any
> idea so far that we should update somewhere in the doc.

I had a look as well and can't really spot anywhere where the current behavior
is detailed, so there is little to update. On top of that, I also don't think
it's worth adding this to the docs.

>> And, I was wondering if it was worth trying to split up the part that
>> reloads the config file and all of the autovacuum stuff. The reloading
>> of the config file by itself won't actually result in autovacuum workers
>> having updated cost delays because of them overwriting it with
>> wi_cost_delay, but it will allow VACUUM to have those updated values.
>
> It makes sense to me to have changes for overhauling the rebalance
> mechanism in a separate patch.

It would for sure be worth considering,

+bool VacuumFailsafeActive = false;
This needs documentation, how it's used and how it relates to failsafe_active
in LVRelState (which it might replace(?), but until then).

+ pg_atomic_uint32 nworkers_for_balance;
This needs a short oneline documentation update to the struct comment.

- double wi_cost_delay;
- int wi_cost_limit;
- int wi_cost_limit_base;
This change makes the below comment in do_autovacuum in need of an update:
/*
* Remove my info from shared memory. We could, but intentionally.
* don't, clear wi_cost_limit and friends --- this is on the
* assumption that we probably have more to do with similar cost
* settings, so we don't want to give up our share of I/O for a very
* short interval and thereby thrash the global balance.
*/

+ if (av_table_option_cost_delay >= 0)
+ VacuumCostDelay = av_table_option_cost_delay;
+ else
+ VacuumCostDelay = autovacuum_vac_cost_delay >= 0 ?
+ autovacuum_vac_cost_delay : VacuumCostDelay;
While it's a matter of personal preference, I for one would like if we reduced
the number of ternary operators in the vacuum code, especially those mixed into
if statements. The vacuum code is full of this already though so this isn't
less of an objection (as it follows style) than an observation.

+ * note: in cost_limit, zero also means use value from elsewhere, because
+ * zero is not a valid value.
...
+ int vac_cost_limit = autovacuum_vac_cost_limit > 0 ?
+ autovacuum_vac_cost_limit : VacuumCostLimit;
Not mentioning the fact that a magic value in a GUC means it's using the value
from another GUC (which is not great IMHO), it seems we are using zero as well
as -1 as that magic value here? (not introduced in this patch.) The docs does
AFAICT only specify -1 as that value though. Am I missing something or is the
code and documentation slightly out of sync?

I need another few readthroughs to figure out of VacuumFailsafeActive does what
I think it does, and should be doing, but in general I think this is a good
idea and a patch in good condition close to being committable.

--
Daniel Gustafsson