> 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
