Re: Should vacuum process config file reload more often - Mailing list pgsql-hackers

On Fri, Apr 7, 2023 at 8:08 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 7 Apr 2023, at 00:12, Melanie Plageman <melanieplageman@gmail.com> wrote:
> >
> > On Thu, Apr 6, 2023 at 5:45 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> >>
> >>> On 6 Apr 2023, at 23:06, Melanie Plageman <melanieplageman@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