On Fri, Apr 7, 2023 at 8:08 AM Daniel Gustafsson <dan...@yesql.se> wrote: > > > On 7 Apr 2023, at 00:12, Melanie Plageman <melanieplage...@gmail.com> wrote: > > > > On Thu, Apr 6, 2023 at 5:45 PM Daniel Gustafsson <dan...@yesql.se> wrote: > >> > >>> On 6 Apr 2023, at 23:06, Melanie Plageman <melanieplage...@gmail.com> > >>> wrote: > >> > >>> Autovacuum workers, at the end of VacuumUpdateCosts(), check if cost > >>> limit or cost delay have been changed. If they have, they assert that > >>> they don't already hold the AutovacuumLock, take it in shared mode, and > >>> do the logging. > >> > >> Another idea would be to copy the values to local temp variables while > >> holding > >> the lock, and release the lock before calling elog() to avoid holding the > >> lock > >> over potential IO. > > > > Good idea. I've done this in attached v19. > > Also I looked through the docs and everything still looks correct for > > balancing algo. > > 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)
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.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
fix.patch
Description: Binary data
use_message_level_is_interesting.patch
Description: Binary data
