> On 7 Apr 2023, at 08:52, Masahiko Sawada <sawada.m...@gmail.com> wrote: > On Fri, Apr 7, 2023 at 8:08 AM Daniel Gustafsson <dan...@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
