Re: Should vacuum process config file reload more often

> On 7 Apr 2023, at 08:52, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Fri, Apr 7, 2023 at 8:08 AM Daniel Gustafsson <daniel@yesql.se> wrote:

>> I had another read-through and test-through of this version, and have applied
>> it with some minor changes to comments and whitespace.  Thanks for the quick
>> turnaround times on reviews in this thread!
> 
> Cool!
> 
> Regarding the commit 7d71d3dd08, I have one comment:
> 
> +       /* Only log updates to cost-related variables */
> +       if (vacuum_cost_delay == original_cost_delay &&
> +           vacuum_cost_limit == original_cost_limit)
> +           return;
> 
> IIUC by default, we log not only before starting the vacuum but also
> when changing cost-related variables. Which is good, I think, because
> logging the initial values would also be helpful for investigation.
> However, I think that we don't log the initial vacuum cost values
> depending on the values. For example, if the
> autovacuum_vacuum_cost_delay storage option is set to 0, we don't log
> the initial values. I think that instead of comparing old and new
> values, we can write the log only if
> message_level_is_interesting(DEBUG2) is true. That way, we don't need
> to acquire the lwlock unnecessarily. And the code looks cleaner to me.
> I've attached the patch (use_message_level_is_interesting.patch)

That's a good idea, unless Melanie has conflicting opinions I think we should
go ahead with this.  Avoiding taking a lock here is a good save.

> Also, while testing the autovacuum delay with relopt
> autovacuum_vacuum_cost_delay = 0, I realized that even if we set
> autovacuum_vacuum_cost_delay = 0 to a table, wi_dobalance is set to
> true. wi_dobalance comes from the following expression:
> 
>        /*
>         * 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));
> 
> The initial values of both avopts->vacuum_cost_limit and
> avopts->vacuum_cost_delay are -1. I think we should use ">= 0" instead
> of "> 0". Otherwise, we include the autovacuum worker working on a
> table whose autovacuum_vacuum_cost_delay is 0 to the balancing
> algorithm. Probably this behavior has existed also on back branches
> but I haven't checked it yet.

Interesting, good find.  Looking quickly at the back branches I think there is
a variant of this for vacuum_cost_limit even there but needs more investigation.

--
Daniel Gustafsson