Subject: Re: Should vacuum process config file reload more often

From: Masahiko Sawada <[email-masked]>
Date: Wed, 15 Mar 2023 14:13:24 +0900

On Sat, Mar 11, 2023 at 8:11 AM Melanie Plageman
<[email-masked]> wrote:
>
> Quotes below are combined from two of Sawada-san's emails.
>
> I've also attached a patch with my suggested current version.
>
> On Thu, Mar 9, 2023 at 10:27 PM Masahiko Sawada <[email-masked]> wrote:
> >
> > On Fri, Mar 10, 2023 at 11:23 AM Melanie Plageman
> > <[email-masked]> wrote:
> > >
> > > On Tue, Mar 7, 2023 at 12:10 AM Masahiko Sawada <[email-masked]> wrote:
> > > >
> > > > On Mon, Mar 6, 2023 at 5:26 AM Melanie Plageman
> > > > <[email-masked]> wrote:
> > > > >
> > > > > On Thu, Mar 2, 2023 at 6:37 PM Melanie Plageman
> > > > > In this version I've removed wi_cost_delay from WorkerInfoData. There is
> > > > > no synchronization of cost_delay amongst workers, so there is no reason
> > > > > to keep it in shared memory.
> > > > >
> > > > > One consequence of not updating VacuumCostDelay from wi_cost_delay is
> > > > > that we have to have a way to keep track of whether or not autovacuum
> > > > > table options are in use.
> > > > >
> > > > > This patch does this in a cringeworthy way. I added two global
> > > > > variables, one to track whether or not cost delay table options are in
> > > > > use and the other to store the value of the table option cost delay. I
> > > > > didn't want to use a single variable with a special value to indicate
> > > > > that table option cost delay is in use because
> > > > > autovacuum_vacuum_cost_delay already has special values that mean
> > > > > certain things. My code needs a better solution.
> > > >
> > > > While it's true that wi_cost_delay doesn't need to be shared, it seems
> > > > to make the logic somewhat complex. We need to handle cost_delay in a
> > > > different way from other vacuum-related parameters and we need to make
> > > > sure av[_use]_table_option_cost_delay are set properly. Removing
> > > > wi_cost_delay from WorkerInfoData saves 8 bytes shared memory per
> > > > autovacuum worker but it might be worth considering to keep
> > > > wi_cost_delay for simplicity.
> > >
> > > Ah, it turns out we can't really remove wi_cost_delay from WorkerInfo
> > > anyway because the launcher doesn't know anything about table options
> > > and so the workers have to keep an updated wi_cost_delay that the
> > > launcher or other autovac workers who are not vacuuming that table can
> > > read from when calculating the new limit in autovac_balance_cost().
> >
> > IIUC if any of the cost delay parameters has been set individually,
> > the autovacuum worker is excluded from the balance algorithm.
>
> Ah, yes! That's right. So it is not a problem. Then I still think
> removing wi_cost_delay from the worker info makes sense. wi_cost_delay
> is a double and can't easily be accessed atomically the way
> wi_cost_limit can be.
>
> Keeping the cost delay local to the backends also makes it clear that
> cost delay is not something that should be written to by other backends
> or that can differ from worker to worker. Without table options in the
> picture, the cost delay should be the same for any worker who has
> reloaded the config file.

Agreed.

>
> As for the cost limit safe access issue, maybe we can avoid a LWLock
> acquisition for reading wi_cost_limit by using an atomic similar to what
> you suggested here for "did_rebalance".
>
> > > I've added in a shared lock for reading from wi_cost_limit in this
> > > patch. However, AutoVacuumUpdateLimit() is called unconditionally in
> > > vacuum_delay_point(), which is called quite often (per block-ish), so I
> > > was trying to think if there is a way we could avoid having to check
> > > this shared memory variable on every call to vacuum_delay_point().
> > > Rebalances shouldn't happen very often (done by the launcher when a new
> > > worker is launched and by workers between vacuuming tables). Maybe we
> > > can read from it less frequently?
> >
> > Yeah, acquiring the lwlock for every call to vacuum_delay_point()
> > seems to be harmful. One idea would be to have one sig_atomic_t
> > variable in WorkerInfoData and autovac_balance_cost() set it to true
> > after rebalancing the worker's cost-limit. The worker can check it
> > without locking and update its delay parameters if the flag is true.
>
> Instead of having the atomic indicate whether or not someone (launcher
> or another worker) did a rebalance, it would simply store the current
> cost limit. Then the worker can normally access it with a simple read.
>
> My rationale is that if we used an atomic to indicate whether or not we
> did a rebalance ("did_rebalance"), we would have the same cache
> coherency guarantees as if we just used the atomic for the cost limit.
> If we read from the "did_rebalance" variable and missed someone having
> written to it on another core, we still wouldn't get around to checking
> the wi_cost_limit variable in shared memory, so it doesn't matter that
> we bothered to keep it in shared memory and use a lock to access it.
>
> I noticed we don't allow wi_cost_limit to ever be less than 0, so we
> could store wi_cost_limit in an atomic uint32.
>
> I'm not sure if it is okay to do pg_atomic_read_u32() and
> pg_atomic_unlocked_write_u32() or if we need pg_atomic_write_u32() in
> most cases.

I agree to use pg_atomic_uin32. Given that the comment of
pg_atomic_unlocked_write_u32() says:

 * pg_atomic_compare_exchange_u32.  This should only be used in cases where
 * minor performance regressions due to atomics emulation are unacceptable.

I think pg_atomic_write_u32() is enough for our use case.

>
> I've implemented the atomic cost limit in the attached patch. Though,
> I'm pretty unsure about how I initialized the atomics in
> AutoVacuumShmemInit()...

+
                 /* initialize the WorkerInfo free list */
                 for (i = 0; i < autovacuum_max_workers; i++)
                         dlist_push_head(&AutoVacuumShmem->av_freeWorkers,
                                                         &worker[i].wi_links);
+
+                dlist_foreach(iter, &AutoVacuumShmem->av_freeWorkers)
+                        pg_atomic_init_u32(
+
&(dlist_container(WorkerInfoData, wi_links, iter.cur))->wi_cost_limit,
+                                                           0);
+

I think we can do like:

    /* initialize the WorkerInfo free list */
    for (i = 0; i < autovacuum_max_workers; i++)
    {
        dlist_push_head(&AutoVacuumShmem->av_freeWorkers,
                                        &worker[i].wi_links);
        pg_atomic_init_u32(&(worker[i].wi_cost_limit));
    }

>
> If the consensus is that it is simply too confusing to take
> wi_cost_delay out of WorkerInfo, we might be able to afford using a
> shared lock to access it because we won't call AutoVacuumUpdateDelay()
> on every invocation of vacuum_delay_point() -- only when we've reloaded
> the config file.
>
> One potential option to avoid taking a shared lock on every call to
> AutoVacuumUpdateDelay() is to set a global variable to indicate that we
> did update it (since we are the only ones updating it) and then only
> take the shared LWLock in AutoVacuumUpdateDelay() if that flag is true.
>

If we remove wi_cost_delay from WorkerInfo, probably we don't need to
acquire the lwlock in AutoVacuumUpdateDelay()? The shared field we
access in that function will be only wi_dobalance, but this field is
updated only by its owner autovacuum worker.

> > ---
> >  void
> >  AutoVacuumUpdateDelay(void)
> >  {
> > -        if (MyWorkerInfo)
> > +        /*
> > +         * We are using autovacuum-related GUCs to update
> > VacuumCostDelay, so we
> > +         * only want autovacuum workers and autovacuum launcher to do this.
> > +         */
> > +        if (!(am_autovacuum_worker || am_autovacuum_launcher))
> > +                return;
> >
> > Is there any case where the autovacuum launcher calls
> > AutoVacuumUpdateDelay() function?
>
> I had meant to add it to HandleAutoVacLauncherInterrupts() after
> reloading the config file (done in attached patch). When using the
> global variables for cost delay (instead of wi_cost_delay in worker
> info), the autovac launcher also has to do the check in the else branch
> of AutoVacuumUpdateDelay()
>
>         VacuumCostDelay = autovacuum_vac_cost_delay >= 0 ?
>             autovacuum_vac_cost_delay : VacuumCostDelay;
>
> to make sure VacuumCostDelay is correct for when it calls
> autovac_balance_cost().

But doesn't the launcher do a similar thing at the beginning of
autovac_balance_cost()?

    double      vac_cost_delay = (autovacuum_vac_cost_delay >= 0 ?
                                  autovacuum_vac_cost_delay : VacuumCostDelay);

Related to this point, I think autovac_balance_cost() should use
globally-set cost_limit and cost_delay values to calculate worker's
vacuum-delay parameters. IOW, vac_cost_limit and vac_cost_delay should
come from the config file setting, not table option etc:

    int         vac_cost_limit = (autovacuum_vac_cost_limit > 0 ?
                                  autovacuum_vac_cost_limit : VacuumCostLimit);
    double      vac_cost_delay = (autovacuum_vac_cost_delay >= 0 ?
                                  autovacuum_vac_cost_delay : VacuumCostDelay);

If my understanding is right, the following change is not right;
AutoVacUpdateLimit() updates the VacuumCostLimit based on the value in
MyWorkerInfo:

                 MyWorkerInfo->wi_cost_limit_base = tab->at_vacuum_cost_limit;
+                AutoVacuumUpdateLimit();

                 /* do a balance */
                 autovac_balance_cost();

-                /* set the active cost parameters from the result of that */
-                AutoVacuumUpdateDelay();

Also, even when using the global variables for cost delay, the
launcher doesn't need to check the global variable. It should always
be able to use either autovacuum_vac_cost_delay/limit or
VacuumCostDelay/Limit.

>
> This also made me think about whether or not we still need cost_limit_base.
> It is used to ensure that autovac_balance_cost() never ends up setting
> workers' wi_cost_limits above the current autovacuum_vacuum_cost_limit
> (or VacuumCostLimit). However, the launcher and all the workers should
> know what the value is without cost_limit_base, no?

Yeah, the current balancing algorithm looks to respect the cost_limit
value set when starting to vacuum the table. The proportion of the
amount of I/O that a worker can consume is calculated based on the
base value and the new worker's cost_limit value cannot exceed the
base value. Given that we're trying to dynamically tune worker's cost
parameters (delay and limit), this concept seems to need to be
updated.

>
> > > Also not sure how the patch interacts with failsafe autovac and parallel
> > > vacuum.
> >
> > Good point.
> >
> > When entering the failsafe mode, we disable the vacuum delays (see
> > lazy_check_wraparound_failsafe()). We need to keep disabling the
> > vacuum delays even after reloading the config file. One idea is to
> > have another global variable indicating we're in the failsafe mode.
> > vacuum_delay_point() doesn't update VacuumCostActive if the flag is
> > true.
>
> I think we might not need to do this. Other than in
> lazy_check_wraparound_failsafe(), VacuumCostActive is only updated in
> two places:
>
> 1) in vacuum() which autovacuum will call per table. And failsafe is
> reset per table as well.
>
> 2) in vacuum_delay_point(), but, since VacuumCostActive will already be
> false when we enter vacuum_delay_point() the next time after
> lazy_check_wraparound_failsafe(), we won't set VacuumCostActive there.

Indeed. But does it mean that there is no code path to turn
vacuum-delay on, even when vacuum_cost_delay is updated from 0 to
non-0?

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

---

From: Melanie Plageman <[email-masked]>
Date: Sat, 18 Mar 2023 18:47:07 -0400

>         pg_atomic_init_u32(&(worker[i].wi_cost_limit));

Ah, yes, I was distracted by the variable name "worker" (as opposed to
"workers").

> updated only by its owner autovacuum worker.

I realized that we cannot use dobalance to decide whether or not to
update wi_cost_delay because dobalance could be false because of table
option cost limit being set (with no table option cost delay) and we
would still need to update VacuumCostDelay and wi_cost_delay with the
new value of autovacuum_vacuum_cost_delay.

But v5 skirts around this issue altogether.

>                                   autovacuum_vac_cost_delay : VacuumCostDelay);

Ah, yes. You are right.

> be able to use either autovacuum_vac_cost_delay/limit or

Yes, that is true. But, I actually think we can do something more
radical, which relates to this point as well as the issue with
cost_limit_base below.

> parameters (delay and limit), this concept seems to need to be

In master, autovacuum workers reload the config file at most once per
table vacuumed. And that is the same time that they update their
wi_cost_limit_base and wi_cost_delay. Thus, when autovac_balance_cost()
is called, there is a good chance that different workers will have
different values for wi_cost_limit_base and wi_cost_delay (and we are
only talking about workers not vacuuming a table with table option
cost-related gucs). So, it made sense that the balancing algorithm tried
to use a ratio to determine what to set the cost limit of each worker
to. It is clamped to the base value, as you say, but it also gives
workers a proportion of the new limit equal to what proportion their base
cost represents of the total cost.

I think all of this doesn't matter anymore now that everyone can reload
the config file often and dynamically change these values.

Thus, in the attached v5, I have removed both wi_cost_limit and wi_cost_delay
from WorkerInfo. I've added a new variable to AutoVacuumShmem called
nworkers_for_balance. Now, autovac_balance_cost() only recalculates this
number and updates it if it has changed. Then, in
AutoVacuumUpdateLimit() workers read from this atomic value and divide
the value of the cost limit gucs by that number to get their own cost limit.

I keep the table option value of cost limit and cost delay in
backend-local memory to reference when updating the worker cost limit.

One nice thing is autovac_balance_cost() only requires an access shared
lock now (though most callers are updating other members before calling
it and still take an exclusive lock).

What do you think?

> vacuum-delay on, even when vacuum_cost_delay is updated from 0 to

Ah yes! Good point. This is true.
I'm not sure how to cheaply allow for re-enabling delays after disabling
them in the middle of a table vacuum.

I don't see a way around checking if we need to reload the config file
on every call to vacuum_delay_point() (currently, we are only doing this
when we have to wait anyway). It seems expensive to do this check every
time. If we do do this, we would update VacuumCostActive when updating
VacuumCostDelay, and we would need a global variable keeping the
failsafe status, as you mentioned.

It could be okay to say that you can only disable cost-based delays in
the middle of vacuuming a table (i.e. you cannot enable them if they are
already disabled until you start vacuuming the next table). Though maybe
it is weird that you can increase the delay but not re-enable it...

On an unrelated note, I was wondering if there were any docs anywhere
that should be updated to go along with this.

And, I was wondering if it was worth trying to split up the part that
reloads the config file and all of the autovacuum stuff. The reloading
of the config file by itself won't actually result in autovacuum workers
having updated cost delays because of them overwriting it with
wi_cost_delay, but it will allow VACUUM to have those updated values.

- Melanie
From f77dc11eb38c96efe8a0defd8cb89ac44481d8f5 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Sat, 18 Mar 2023 15:42:39 -0400
Subject: [PATCH v5] [auto]vacuum reloads config file more often

Previously, VACUUM and autovacuum workers would reload the configuration
file only between vacuuming tables. This precluded user updates to
cost-based delay parameters from taking effect while vacuuming a table.

Check if a reload is pending roughly once per block now, when checking
if we need to delay.

In order for this change to have the intended effect on autovacuum,
autovacuum workers must start updating their cost delay more frequently
as well.

With this new paradigm, balancing the cost limit amongst workers also
must work differently. Previously, a worker's wi_cost_limit was set only
at the beginning of vacuuming a table, after reloading the config file.
Therefore, at the time that autovac_balance_cost() is called, workers
vacuuming tables with no table options could still have different values
for their wi_cost_limit_base and wi_cost_delay. With this change,
workers will (within some margin of error) have no reason to have
different values for cost limit and cost delay (in the absence of table
options). Thus, remove cost limit and cost delay from shared memory and
keep track only of the number of workers actively vacuuming tables with
no cost-related table options. Then, use this value to determine what
each worker's effective cost limit should be.

Reviewed-by: Masahiko Sawada <[email-masked]>
Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_buP5wzsho3qNw5o9_R0pF69FRM5hgCmr-mvXmGXwdA7A%40mail.gmail.com#5e6771d4cdca4db6efc2acec2dce0bc7
---
 src/backend/commands/vacuum.c       |  40 ++++--
 src/backend/postmaster/autovacuum.c | 206 ++++++++++++----------------
 src/include/postmaster/autovacuum.h |   2 +
 3 files changed, 121 insertions(+), 127 deletions(-)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index c54360a6a0..6534fd748d 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -48,6 +48,7 @@
 #include "pgstat.h"
 #include "postmaster/autovacuum.h"
 #include "postmaster/bgworker_internals.h"
+#include "postmaster/interrupt.h"
 #include "storage/bufmgr.h"
 #include "storage/lmgr.h"
 #include "storage/pmsignal.h"
@@ -76,6 +77,7 @@ int			vacuum_multixact_failsafe_age;
 /* A few variables that don't seem worth passing around as parameters */
 static MemoryContext vac_context = NULL;
 static BufferAccessStrategy vac_strategy;
+static bool analyze_in_outer_xact = false;

 /*
@@ -314,8 +316,7 @@ vacuum(List *relations, VacuumParams *params,
 	static bool in_vacuum = false;
 
 	const char *stmttype;
-	volatile bool in_outer_xact,
-				use_own_xacts;
+	volatile bool use_own_xacts;
 
 	Assert(params != NULL);
 
@@ -332,10 +333,10 @@ vacuum(List *relations, VacuumParams *params,
 	if (params->options & VACOPT_VACUUM)
 	{
 		PreventInTransactionBlock(isTopLevel, stmttype);
-		in_outer_xact = false;
+		analyze_in_outer_xact = false;
 	}
 	else
-		in_outer_xact = IsInTransactionBlock(isTopLevel);
+		analyze_in_outer_xact = IsInTransactionBlock(isTopLevel);
 
 	/*
 	 * Due to static variables vac_context, anl_context and vac_strategy,
@@ -457,7 +458,7 @@ vacuum(List *relations, VacuumParams *params,
 		Assert(params->options & VACOPT_ANALYZE);
 		if (IsAutoVacuumWorkerProcess())
 			use_own_xacts = true;
-		else if (in_outer_xact)
+		else if (analyze_in_outer_xact)
 			use_own_xacts = false;
 		else if (list_length(relations) > 1)
 			use_own_xacts = true;
@@ -475,7 +476,7 @@ vacuum(List *relations, VacuumParams *params,
 	 */
 	if (use_own_xacts)
 	{
-		Assert(!in_outer_xact);
+		Assert(!analyze_in_outer_xact);
 
 		/* ActiveSnapshot is not set by autovacuum */
 		if (ActiveSnapshotSet())
@@ -527,7 +528,7 @@ vacuum(List *relations, VacuumParams *params,
 				}
 
 				analyze_rel(vrel->oid, vrel->relation, params,
-							vrel->va_cols, in_outer_xact, vac_strategy);
+							vrel->va_cols, analyze_in_outer_xact, vac_strategy);
 
 				if (use_own_xacts)
 				{
@@ -550,6 +551,7 @@ vacuum(List *relations, VacuumParams *params,
 	{
 		in_vacuum = false;
 		VacuumCostActive = false;
+		analyze_in_outer_xact = false;
 	}
 	PG_END_TRY();
 
@@ -2246,10 +2248,30 @@ vacuum_delay_point(void)
 		if (IsUnderPostmaster && !PostmasterIsAlive())
 			exit(1);
 
+		/*
+		 * Reload the configuration file if requested. This allows changes to
+		 * [autovacuum_]vacuum_cost_limit and [autovacuum_]vacuum_cost_delay
+		 * to take effect while a table is being vacuumed or analyzed.
+		 */
+		if (ConfigReloadPending && !analyze_in_outer_xact)
+		{
+			ConfigReloadPending = false;
+			ProcessConfigFile(PGC_SIGHUP);
+			AutoVacuumUpdateDelay();
+		}
+
 		VacuumCostBalance = 0;
 
-		/* update balance values for workers */
-		AutoVacuumUpdateDelay();
+		/*
+		 * Update balance values for workers. We must always do this in case
+		 * the autovacuum launcher or another autovacuum worker has
+		 * recalculated the number of workers across which we must balance the
+		 * limit. This is done by the launcher when launching a new worker and
+		 * by workers before vacuuming each table.
+		 */
+		AutoVacuumUpdateLimit();
+
+		VacuumCostActive = (VacuumCostDelay > 0);
 
 		/* Might have gotten an interrupt while sleeping */
 		CHECK_FOR_INTERRUPTS();
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index c0e2e00a7e..f69f011589 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -139,6 +139,9 @@ int			Log_autovacuum_min_duration = 600000;
 static bool am_autovacuum_launcher = false;
 static bool am_autovacuum_worker = false;
 
+static double av_table_option_cost_delay = -1;
+static int	av_table_option_cost_limit = 0;
+
 /* Flags set by signal handlers */
 static volatile sig_atomic_t got_SIGUSR2 = false;
 
@@ -189,8 +192,8 @@ typedef struct autovac_table
 {
 	Oid			at_relid;
 	VacuumParams at_params;
-	double		at_vacuum_cost_delay;
-	int			at_vacuum_cost_limit;
+	double		at_table_option_vac_cost_delay;
+	int			at_table_option_vac_cost_limit;
 	bool		at_dobalance;
 	bool		at_sharedrel;
 	char	   *at_relname;
@@ -209,7 +212,7 @@ typedef struct autovac_table
  * wi_sharedrel flag indicating whether table is marked relisshared
  * wi_proc		pointer to PGPROC of the running worker, NULL if not started
  * wi_launchtime Time at which this worker was launched
- * wi_cost_*	Vacuum cost-based delay parameters current in this worker
+ * wi_dobalance Whether this worker should be included in balance calculations
  *
  * All fields are protected by AutovacuumLock, except for wi_tableoid and
  * wi_sharedrel which are protected by AutovacuumScheduleLock (note these
@@ -225,9 +228,6 @@ typedef struct WorkerInfoData
 	TimestampTz wi_launchtime;
 	bool		wi_dobalance;
 	bool		wi_sharedrel;
-	double		wi_cost_delay;
-	int			wi_cost_limit;
-	int			wi_cost_limit_base;
 } WorkerInfoData;
 
 typedef struct WorkerInfoData *WorkerInfo;
@@ -286,6 +286,7 @@ typedef struct
 	dlist_head	av_runningWorkers;
 	WorkerInfo	av_startingWorker;
 	AutoVacuumWorkItem av_workItems[NUM_WORKITEMS];
+	pg_atomic_uint32 nworkers_for_balance;
 } AutoVacuumShmemStruct;
 
 static AutoVacuumShmemStruct *AutoVacuumShmem;
@@ -820,7 +821,7 @@ HandleAutoVacLauncherInterrupts(void)
 			AutoVacLauncherShutdown();
 
 		/* rebalance in case the default cost parameters changed */
-		LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
+		LWLockAcquire(AutovacuumLock, LW_SHARED);
 		autovac_balance_cost();
 		LWLockRelease(AutovacuumLock);
 
@@ -1756,9 +1757,6 @@ FreeWorkerInfo(int code, Datum arg)
 		MyWorkerInfo->wi_proc = NULL;
 		MyWorkerInfo->wi_launchtime = 0;
 		MyWorkerInfo->wi_dobalance = false;
-		MyWorkerInfo->wi_cost_delay = 0;
-		MyWorkerInfo->wi_cost_limit = 0;
-		MyWorkerInfo->wi_cost_limit_base = 0;
 		dlist_push_head(&AutoVacuumShmem->av_freeWorkers,
 						&MyWorkerInfo->wi_links);
 		/* not mine anymore */
@@ -1774,99 +1772,95 @@ FreeWorkerInfo(int code, Datum arg)
 }
 
 /*
- * Update the cost-based delay parameters, so that multiple workers consume
- * each a fraction of the total available I/O.
+ * Update VacuumCostDelay with the correct value for an autovacuum worker,
+ * given the value of other relevant cost-based delay parameters. Autovacuum
+ * workers should call this after every config reload, in case VacuumCostDelay
+ * was overwritten.
  */
 void
 AutoVacuumUpdateDelay(void)
 {
-	if (MyWorkerInfo)
+	if (!am_autovacuum_worker)
+		return;
+
+	if (av_table_option_cost_delay >= 0)
+		VacuumCostDelay = av_table_option_cost_delay;
+	else
+		VacuumCostDelay = autovacuum_vac_cost_delay >= 0 ?
+			autovacuum_vac_cost_delay : VacuumCostDelay;
+}
+
+/*
+ * Update VacuumCostLimit with the correct value for an autovacuum worker,
+ * given the value of other relevant cost limit parameters and the number of
+ * workers across which the limit must be balanced. Autovacuum workers must
+ * call this regularly in case nworkers_for_balance has been updated by another
+ * worker or by the autovacuum launcher. They also must call this after every
+ * config reload, in case VacuumCostLimit was overwritten.
+ */
+void
+AutoVacuumUpdateLimit(void)
+{
+	if (!am_autovacuum_worker)
+		return;
+
+	/*
+	 * note: in cost_limit, zero also means use value from elsewhere, because
+	 * zero is not a valid value.
+	 */
+	if (av_table_option_cost_limit > 0)
+		VacuumCostLimit = av_table_option_cost_limit;
+	else
 	{
-		VacuumCostDelay = MyWorkerInfo->wi_cost_delay;
-		VacuumCostLimit = MyWorkerInfo->wi_cost_limit;
+		int			vac_cost_limit = autovacuum_vac_cost_limit > 0 ?
+		autovacuum_vac_cost_limit : VacuumCostLimit;
+
+		int			balanced_cost_limit = vac_cost_limit /
+		pg_atomic_read_u32(&AutoVacuumShmem->nworkers_for_balance);
+
+		VacuumCostLimit = Max(Min(balanced_cost_limit, vac_cost_limit), 1);
 	}
 }
 
 /*
  * autovac_balance_cost
- *		Recalculate the cost limit setting for each active worker.
+ *		Recalculate the number of workers to consider, given table options and
+ *		the current number of active workers.
  *
- * Caller must hold the AutovacuumLock in exclusive mode.
+ * Caller must hold the AutovacuumLock in at least shared mode.
  */
 static void
 autovac_balance_cost(void)
 {
-	/*
-	 * The idea here is that we ration out I/O equally.  The amount of I/O
-	 * that a worker can consume is determined by cost_limit/cost_delay, so we
-	 * try to equalize those ratios rather than the raw limit settings.
-	 *
-	 * note: in cost_limit, zero also means use value from elsewhere, because
-	 * zero is not a valid value.
-	 */
-	int			vac_cost_limit = (autovacuum_vac_cost_limit > 0 ?
-								  autovacuum_vac_cost_limit : VacuumCostLimit);
-	double		vac_cost_delay = (autovacuum_vac_cost_delay >= 0 ?
-								  autovacuum_vac_cost_delay : VacuumCostDelay);
-	double		cost_total;
-	double		cost_avail;
 	dlist_iter	iter;
+	int			orig_nworkers_for_balance;
+	int			nworkers_for_balance = 0;
 
-	/* not set? nothing to do */
-	if (vac_cost_limit <= 0 || vac_cost_delay <= 0)
+	if (autovacuum_vac_cost_delay == 0 ||
+		(autovacuum_vac_cost_delay == -1 && VacuumCostDelay == 0))
 		return;
 
-	/* calculate the total base cost limit of participating active workers */
-	cost_total = 0.0;
-	dlist_foreach(iter, &AutoVacuumShmem->av_runningWorkers)
-	{
-		WorkerInfo	worker = dlist_container(WorkerInfoData, wi_links, iter.cur);
-
-		if (worker->wi_proc != NULL &&
-			worker->wi_dobalance &&
-			worker->wi_cost_limit_base > 0 && worker->wi_cost_delay > 0)
-			cost_total +=
-				(double) worker->wi_cost_limit_base / worker->wi_cost_delay;
-	}
-
-	/* there are no cost limits -- nothing to do */
-	if (cost_total <= 0)
+	if (autovacuum_vac_cost_limit <= 0 && VacuumCostLimit <= 0)
 		return;
 
-	/*
-	 * Adjust cost limit of each active worker to balance the total of cost
-	 * limit to autovacuum_vacuum_cost_limit.
-	 */
-	cost_avail = (double) vac_cost_limit / vac_cost_delay;
+	orig_nworkers_for_balance =
+		pg_atomic_read_u32(&AutoVacuumShmem->nworkers_for_balance);
+
 	dlist_foreach(iter, &AutoVacuumShmem->av_runningWorkers)
 	{
 		WorkerInfo	worker = dlist_container(WorkerInfoData, wi_links, iter.cur);
 
-		if (worker->wi_proc != NULL &&
-			worker->wi_dobalance &&
-			worker->wi_cost_limit_base > 0 && worker->wi_cost_delay > 0)
-		{
-			int			limit = (int)
-			(cost_avail * worker->wi_cost_limit_base / cost_total);
-
-			/*
-			 * We put a lower bound of 1 on the cost_limit, to avoid division-
-			 * by-zero in the vacuum code.  Also, in case of roundoff trouble
-			 * in these calculations, let's be sure we don't ever set
-			 * cost_limit to more than the base value.
-			 */
-			worker->wi_cost_limit = Max(Min(limit,
-											worker->wi_cost_limit_base),
-										1);
-		}
+		if (worker->wi_proc == NULL || !worker->wi_dobalance)
+			continue;
 
-		if (worker->wi_proc != NULL)
-			elog(DEBUG2, "autovac_balance_cost(pid=%d db=%u, rel=%u, dobalance=%s cost_limit=%d, cost_limit_base=%d, cost_delay=%g)",
-				 worker->wi_proc->pid, worker->wi_dboid, worker->wi_tableoid,
-				 worker->wi_dobalance ? "yes" : "no",
-				 worker->wi_cost_limit, worker->wi_cost_limit_base,
-				 worker->wi_cost_delay);
+		nworkers_for_balance++;
 	}
+
+	nworkers_for_balance = Max(nworkers_for_balance, 1);
+
+	if (nworkers_for_balance != orig_nworkers_for_balance)
+		pg_atomic_write_u32(&AutoVacuumShmem->nworkers_for_balance,
+							nworkers_for_balance);
 }
 
 /*
@@ -2312,14 +2306,15 @@ do_autovacuum(void)
 		autovac_table *tab;
 		bool		isshared;
 		bool		skipit;
-		double		stdVacuumCostDelay;
-		int			stdVacuumCostLimit;
 		dlist_iter	iter;
 
 		CHECK_FOR_INTERRUPTS();
 
 		/*
 		 * Check for config changes before processing each collected table.
+		 * Autovacuum workers must update VacuumCostDelay and VacuumCostLimit
+		 * in case they were overridden by the reload. However, we will do
+		 * this as soon as we check table options a bit later.
 		 */
 		if (ConfigReloadPending)
 		{
@@ -2416,32 +2411,18 @@ do_autovacuum(void)
 			continue;
 		}
 
-		/*
-		 * Remember the prevailing values of the vacuum cost GUCs.  We have to
-		 * restore these at the bottom of the loop, else we'll compute wrong
-		 * values in the next iteration of autovac_balance_cost().
-		 */
-		stdVacuumCostDelay = VacuumCostDelay;
-		stdVacuumCostLimit = VacuumCostLimit;
+		av_table_option_cost_limit = tab->at_table_option_vac_cost_limit;
+		av_table_option_cost_delay = tab->at_table_option_vac_cost_delay;
 
 		/* Must hold AutovacuumLock while mucking with cost balance info */
 		LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
-
-		/* advertise my cost delay parameters for the balancing algorithm */
 		MyWorkerInfo->wi_dobalance = tab->at_dobalance;
-		MyWorkerInfo->wi_cost_delay = tab->at_vacuum_cost_delay;
-		MyWorkerInfo->wi_cost_limit = tab->at_vacuum_cost_limit;
-		MyWorkerInfo->wi_cost_limit_base = tab->at_vacuum_cost_limit;
-
-		/* do a balance */
 		autovac_balance_cost();
+		LWLockRelease(AutovacuumLock);
 
 		AutoVacuumUpdateDelay();
 
-		/* done */
-		LWLockRelease(AutovacuumLock);
-
 		/* clean up memory before each iteration */
 		MemoryContextResetAndDeleteChildren(PortalContext);
 
@@ -2534,10 +2515,6 @@ deleted:
 		MyWorkerInfo->wi_tableoid = InvalidOid;
 		MyWorkerInfo->wi_sharedrel = false;
 		LWLockRelease(AutovacuumScheduleLock);
-
-		/* restore vacuum cost GUCs for the next iteration */
-		VacuumCostDelay = stdVacuumCostDelay;
-		VacuumCostLimit = stdVacuumCostLimit;
 	}
 
 	/*
@@ -2569,6 +2546,8 @@ deleted:
 		{
 			ConfigReloadPending = false;
 			ProcessConfigFile(PGC_SIGHUP);
+			AutoVacuumUpdateDelay();
+			AutoVacuumUpdateLimit();
 		}
 
 		LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
@@ -2801,8 +2780,6 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
 		int			freeze_table_age;
 		int			multixact_freeze_min_age;
 		int			multixact_freeze_table_age;
-		int			vac_cost_limit;
-		double		vac_cost_delay;
 		int			log_min_duration;
 
 		/*
@@ -2812,20 +2789,6 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
 		 * defaults, autovacuum's own first and plain vacuum second.
 		 */
 
-		/* -1 in autovac setting means use plain vacuum_cost_delay */
-		vac_cost_delay = (avopts && avopts->vacuum_cost_delay >= 0)
-			? avopts->vacuum_cost_delay
-			: (autovacuum_vac_cost_delay >= 0)
-			? autovacuum_vac_cost_delay
-			: VacuumCostDelay;
-
-		/* 0 or -1 in autovac setting means use plain vacuum_cost_limit */
-		vac_cost_limit = (avopts && avopts->vacuum_cost_limit > 0)
-			? avopts->vacuum_cost_limit
-			: (autovacuum_vac_cost_limit > 0)
-			? autovacuum_vac_cost_limit
-			: VacuumCostLimit;
-
 		/* -1 in autovac setting means use log_autovacuum_min_duration */
 		log_min_duration = (avopts && avopts->log_min_duration >= 0)
 			? avopts->log_min_duration
@@ -2881,8 +2844,10 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
 		tab->at_params.multixact_freeze_table_age = multixact_freeze_table_age;
 		tab->at_params.is_wraparound = wraparound;
 		tab->at_params.log_min_duration = log_min_duration;
-		tab->at_vacuum_cost_limit = vac_cost_limit;
-		tab->at_vacuum_cost_delay = vac_cost_delay;
+		tab->at_table_option_vac_cost_limit = avopts ?
+			avopts->vacuum_cost_limit : 0;
+		tab->at_table_option_vac_cost_delay = avopts ?
+			avopts->vacuum_cost_delay : -1;
 		tab->at_relname = NULL;
 		tab->at_nspname = NULL;
 		tab->at_datname = NULL;
@@ -3374,10 +3339,15 @@ AutoVacuumShmemInit(void)
 		worker = (WorkerInfo) ((char *) AutoVacuumShmem +
 							   MAXALIGN(sizeof(AutoVacuumShmemStruct)));
 
+
 		/* initialize the WorkerInfo free list */
 		for (i = 0; i < autovacuum_max_workers; i++)
 			dlist_push_head(&AutoVacuumShmem->av_freeWorkers,
 							&worker[i].wi_links);
+
+		/* initialize to 1, as it should be a minimum of 1 */
+		pg_atomic_init_u32(&AutoVacuumShmem->nworkers_for_balance, 1);
+
 	}
 	else
 		Assert(found);
diff --git a/src/include/postmaster/autovacuum.h b/src/include/postmaster/autovacuum.h
index c140371b51..558358911c 100644
--- a/src/include/postmaster/autovacuum.h
+++ b/src/include/postmaster/autovacuum.h
@@ -66,6 +66,8 @@ extern void AutoVacWorkerFailed(void);
 /* autovacuum cost-delay balancer */
 extern void AutoVacuumUpdateDelay(void);
 
+extern void AutoVacuumUpdateLimit(void);
+
 #ifdef EXEC_BACKEND
 extern void AutoVacLauncherMain(int argc, char *argv[]) pg_attribute_noreturn();
 extern void AutoVacWorkerMain(int argc, char *argv[]) pg_attribute_noreturn();
-- 
2.37.2

---

From: Melanie Plageman <[email-masked]>
Date: Sun, 19 Mar 2023 12:48:38 -0400

> it is weird that you can increase the delay but not re-enable it...

So, I thought about it some more, and I think it is a bit odd that you
can increase the delay and limit but not re-enable them if they were
disabled. And, perhaps it would be okay to check ConfigReloadPending at
the top of vacuum_delay_point() instead of only after sleeping. It is
just one more branch. We can check if VacuumCostActive is false after
checking if we should reload and doing so if needed and return early.
I've implemented that in attached v6.

I added in the global we discussed for VacuumFailsafeActive. If we keep
it, we can probably remove the one in LVRelState -- as it seems
redundant. Let me know what you think.

- Melanie
From 1218c1852794a1310d25359a37b87d068282500e Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Sat, 18 Mar 2023 15:42:39 -0400
Subject: [PATCH v6] [auto]vacuum reloads config file more often

Reviewed-by: Masahiko Sawada <[email-masked]>
Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_buP5wzsho3qNw5o9_R0pF69FRM5hgCmr-mvXmGXwdA7A%40mail.gmail.com#5e6771d4cdca4db6efc2acec2dce0bc7
---
 src/backend/access/heap/vacuumlazy.c  |   1 +
 src/backend/commands/vacuum.c         |  55 +++++--
 src/backend/commands/vacuumparallel.c |   1 +
 src/backend/postmaster/autovacuum.c   | 209 +++++++++++---------------
 src/backend/utils/init/globals.c      |   1 +
 src/include/miscadmin.h               |   1 +
 src/include/postmaster/autovacuum.h   |   2 +
 7 files changed, 142 insertions(+), 128 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 8f14cf85f3..38dce9ae66 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -2622,6 +2622,7 @@ lazy_check_wraparound_failsafe(LVRelState *vacrel)
 	if (unlikely(vacuum_xid_failsafe_check(&vacrel->cutoffs)))
 	{
 		vacrel->failsafe_active = true;
+		VacuumFailsafeActive = true;
 
 		/* Disable index vacuuming, index cleanup, and heap rel truncation */
 		vacrel->do_index_vacuuming = false;
index c54360a6a0..a5eb22c5ca 100644

 /*
 
 	const char *stmttype;
 
 	Assert(params != NULL);

 	/*
 
 		/* ActiveSnapshot is not set by autovacuum */
 		if (ActiveSnapshotSet())
@@ -492,6 +493,7 @@ vacuum(List *relations, VacuumParams *params,
 
 		in_vacuum = true;
 		VacuumCostActive = (VacuumCostDelay > 0);
+		VacuumFailsafeActive = false;
 		VacuumCostBalance = 0;
 		VacuumPageHit = 0;
 		VacuumPageMiss = 0;
@@ -527,7 +529,7 @@ vacuum(List *relations, VacuumParams *params,
 				}

 				if (use_own_xacts)
 				{
@@ -550,6 +552,7 @@ vacuum(List *relations, VacuumParams *params,
 	{
 		in_vacuum = false;
 		VacuumCostActive = false;
+		analyze_in_outer_xact = false;
 	}
 	PG_END_TRY();
 
@@ -1850,6 +1853,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params, bool skip_privs)
 
 	Assert(params != NULL);
 
+	VacuumFailsafeActive = false;
+
 	/* Begin a transaction for vacuuming this relation */
 	StartTransactionCommand();
 
@@ -2215,9 +2220,33 @@ vacuum_delay_point(void)
 	/* Always check for interrupts */
 	CHECK_FOR_INTERRUPTS();
 
-	if (!VacuumCostActive || InterruptPending)
+	if (InterruptPending || VacuumFailsafeActive ||
+		(!VacuumCostActive && !ConfigReloadPending))
 		return;
 
+	/*
+	 * Reload the configuration file if requested. This allows changes to
+	 * [autovacuum_]vacuum_cost_limit and [autovacuum_]vacuum_cost_delay to
+	 * take effect while a table is being vacuumed or analyzed.
+	 */
+	if (ConfigReloadPending && !analyze_in_outer_xact)
+	{
+		ConfigReloadPending = false;
+		ProcessConfigFile(PGC_SIGHUP);
+		AutoVacuumUpdateDelay();
+		AutoVacuumUpdateLimit();
+	}
+
+	/*
+	 * If we disabled cost-based delays after reloading the config file,
+	 * return
+	 */
+	if (!VacuumCostActive)
+	{
+		VacuumCostBalance = 0;
+		return;
+	}
+
 	/*
 	 * For parallel vacuum, the delay is computed based on the shared cost
 	 * balance.  See compute_parallel_delay.
@@ -2248,8 +2277,14 @@ vacuum_delay_point(void)
 
 		VacuumCostBalance = 0;
 
-		/* update balance values for workers */
-		AutoVacuumUpdateDelay();
+		/*
+		 * Update limit values for workers. We must always do this in case the
+		 * autovacuum launcher or another autovacuum worker has recalculated
+		 * the number of workers across which we must balance the limit. This
+		 * is done by the launcher when launching a new worker and by workers
+		 * before vacuuming each table.
+		 */
+		AutoVacuumUpdateLimit();
 
 		/* Might have gotten an interrupt while sleeping */
 		CHECK_FOR_INTERRUPTS();
diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c
index bcd40c80a1..57188500d0 100644
--- a/src/backend/commands/vacuumparallel.c
+++ b/src/backend/commands/vacuumparallel.c
@@ -990,6 +990,7 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
 												 false);
 
 	/* Set cost-based vacuum delay */
+	VacuumFailsafeActive = false;
 	VacuumCostActive = (VacuumCostDelay > 0);
 	VacuumCostBalance = 0;
 	VacuumPageHit = 0;
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index c0e2e00a7e..1033e6db62 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -139,6 +139,9 @@ int			Log_autovacuum_min_duration = 600000;
 static bool am_autovacuum_launcher = false;
 static bool am_autovacuum_worker = false;

 

 
@@ -1774,99 +1772,98 @@ FreeWorkerInfo(int code, Datum arg)
 }
 
 /*
+	if (!VacuumFailsafeActive)
 
 /*
  * autovac_balance_cost

 
-	/*

 
 /*
@@ -2312,14 +2309,15 @@ do_autovacuum(void)
 		autovac_table *tab;
 		bool		isshared;
 		bool		skipit;
-		double		stdVacuumCostDelay;
-		int			stdVacuumCostLimit;
 		dlist_iter	iter;
 
 		CHECK_FOR_INTERRUPTS();
 
 		/*
@@ -2416,32 +2414,18 @@ do_autovacuum(void)
 			continue;
 		}
 
-		/*

 
-		/* done */
 
@@ -2534,10 +2518,6 @@ deleted:
 
 	/*
@@ -2569,6 +2549,8 @@ deleted:
 		{
 
 		LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
@@ -2801,8 +2783,6 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
 		int			freeze_table_age;
 		int			multixact_freeze_min_age;
 		int			multixact_freeze_table_age;
-		int			vac_cost_limit;
-		double		vac_cost_delay;
 		int			log_min_duration;
 
 		/*
@@ -2812,20 +2792,6 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
 		 * defaults, autovacuum's own first and plain vacuum second.
 		 */
 
@@ -2881,8 +2847,10 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
@@ -3374,10 +3342,15 @@ AutoVacuumShmemInit(void)
 
+
 		/* initialize the WorkerInfo free list */
 		for (i = 0; i < autovacuum_max_workers; i++)
 			dlist_push_head(&AutoVacuumShmem->av_freeWorkers,
 							&worker[i].wi_links);
+
+		/* initialize to 1, as it should be a minimum of 1 */
+		pg_atomic_init_u32(&AutoVacuumShmem->nworkers_for_balance, 1);
+
 	}
 	else
 		Assert(found);
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 1b1d814254..aeb8ed0e46 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -151,3 +151,4 @@ int64		VacuumPageDirty = 0;
 
 int			VacuumCostBalance = 0;	/* working state for vacuum */
 bool		VacuumCostActive = false;
+bool		VacuumFailsafeActive = false;
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 06a86f9ac1..b1297677d3 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -275,6 +275,7 @@ extern PGDLLIMPORT int64 VacuumPageDirty;
 
 extern PGDLLIMPORT int VacuumCostBalance;
 extern PGDLLIMPORT bool VacuumCostActive;
+extern PGDLLIMPORT bool VacuumFailsafeActive;

 /* in tcop/postgres.c */

---

From: Masahiko Sawada <[email-masked]>
Date: Thu, 23 Mar 2023 15:08:25 +0900

> it and still take an exclusive lock).

I think this is a good idea.

Do we need to calculate the number of workers running with
nworkers_for_balance by iterating over the running worker list? I
guess autovacuum workers can increment/decrement it at the beginning
and end of vacuum.

> redundant. Let me know what you think.

I think the following change is related:

+        /*

It makes sense to me that we need to reload the config file even when
vacuum-delay is disabled. But I think it's not convenient for users
that we don't reload the configuration file once the failsafe is
triggered. I think users might want to change some GUCs such as
log_autovacuum_min_duration.

> that should be updated to go along with this.

The current patch improves the internal mechanism of (re)balancing
vacuum-cost but doesn't change user-visible behavior. I don't have any
idea so far that we should update somewhere in the doc.

> wi_cost_delay, but it will allow VACUUM to have those updated values.

It makes sense to me to have changes for overhauling the rebalance
mechanism in a separate patch.

Looking back at the original concern you mentioned[1]:

speed up long-running vacuum of a large table by
decreasing autovacuum_vacuum_cost_delay/vacuum_cost_delay, however the
config file is only reloaded between tables (for autovacuum) or after
the statement (for explicit vacuum).

does it make sense to have autovac_balance_cost() update workers'
wi_cost_delay too? Autovacuum launcher already reloads the config file
and does the rebalance. So I thought autovac_balance_cost() can update
the cost_delay as well, and this might be a minimal change to deal
with your concern. This doesn't have the effect for manual VACUUM but
since vacuum delay is disabled by default it won't be a big problem.
As for manual VACUUMs, we would need to reload the config file in
vacuum_delay_point() as the part of your patch does. Overhauling the
rebalance mechanism would be another patch to improve it further.

Regards,

[1] https://www.postgresql.org/message-id/CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7%2BDXhrjDA0gw%40mail.gmail.com

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 23 Mar 2023 17:24:36 +0100

> On 23 Mar 2023, at 07:08, Masahiko Sawada <[email-masked]> wrote:
> On Sun, Mar 19, 2023 at 7:47 AM Melanie Plageman <[email-masked]> wrote:

> log_autovacuum_min_duration.

I agree with this.

> idea so far that we should update somewhere in the doc.

I had a look as well and can't really spot anywhere where the current behavior
is detailed, so there is little to update.  On top of that, I also don't think
it's worth adding this to the docs.

> mechanism in a separate patch.

It would for sure be worth considering, 

+bool       VacuumFailsafeActive = false;
This needs documentation, how it's used and how it relates to failsafe_active
in LVRelState (which it might replace(?), but until then).

+   pg_atomic_uint32 nworkers_for_balance;
This needs a short oneline documentation update to the struct comment.

-   double      wi_cost_delay;
-   int         wi_cost_limit;
-   int         wi_cost_limit_base;
This change makes the below comment in do_autovacuum in need of an update:
  /*
   * Remove my info from shared memory.  We could, but intentionally.
   * don't, clear wi_cost_limit and friends --- this is on the
   * assumption that we probably have more to do with similar cost
   * settings, so we don't want to give up our share of I/O for a very
   * short interval and thereby thrash the global balance.
   */

+   if (av_table_option_cost_delay >= 0)
+       VacuumCostDelay = av_table_option_cost_delay;
+   else
+       VacuumCostDelay = autovacuum_vac_cost_delay >= 0 ?
+           autovacuum_vac_cost_delay : VacuumCostDelay;
While it's a matter of personal preference, I for one would like if we reduced
the number of ternary operators in the vacuum code, especially those mixed into
if statements.  The vacuum code is full of this already though so this isn't
less of an objection (as it follows style) than an observation.

+    * note: in cost_limit, zero also means use value from elsewhere, because
+    * zero is not a valid value.
...
+       int         vac_cost_limit = autovacuum_vac_cost_limit > 0 ?
+       autovacuum_vac_cost_limit : VacuumCostLimit;
Not mentioning the fact that a magic value in a GUC means it's using the value
from another GUC (which is not great IMHO), it seems we are using zero as well
as -1 as that magic value here?  (not introduced in this patch.) The docs does
AFAICT only specify -1 as that value though.  Am I missing something or is the
code and documentation slightly out of sync?

I need another few readthroughs to figure out of VacuumFailsafeActive does what
I think it does, and should be doing, but in general I think this is a good
idea and a patch in good condition close to being committable.

--
Daniel Gustafsson

---

From: Melanie Plageman <[email-masked]>
Date: Thu, 23 Mar 2023 20:27:17 -0400

> guess autovacuum workers can increment/decrement it at the beginning

I don't think we can do that because if a worker crashes, we have no way
of knowing if it had incremented or decremented the number, so we can't
adjust for it.

> log_autovacuum_min_duration.

Ah, okay. Attached v7 has this change (it reloads even if failsafe is
active).

> rebalance mechanism would be another patch to improve it further.

So, we can't do this without acquiring an access shared lock on every
call to vacuum_delay_point() because cost delay is a double.

I will work on a patchset with separate commits for reloading the config
file, though (with autovac not benefitting in the first commit).

> in LVRelState (which it might replace(?), but until then).

Thanks! I've removed LVRelState->failsafe_active.

I've also separated the VacuumFailsafeActive change into its own commit.
I will say that that commit message needs some work.

> This needs a short oneline documentation update to the struct comment.

Done. I also prefixed with av to match the other members. I am thinking
that this variable name could be better. I want to convey that it is the
number of workers sharing a cost limit, so I considered
av_limit_sharers or something like that. I am looking to convey that
it is the number of workers amongst whom we must split the cost limit.

>    * short interval and thereby thrash the global balance.

Updated to mention wi_dobalance instead.
On the topic of wi_dobalance, should we bother making it an atomic flag
instead? We would avoid taking a lock a few times, though probably not
frequently enough to matter. I was wondering if making it atomically
accessible would be less confusing than acquiring a lock only to set
one member in do_autovacuum() (and otherwise it is only read). I think
if I had to make it an atomic flag, I would reverse the logic and make
it wi_skip_balance or something like that.

> less of an objection (as it follows style) than an observation.

I agree. This one was better served as an "else if" anyway -- updated!

> code and documentation slightly out of sync?

I copied that comment from elsewhere, but, yes it is a weird situation.
So, you can set autovacuum_vacuum_cost_limit to 0, -1 or a
positive number. You can only set vacuum_cost_limit to a positive value.
The documentation mentions that setting autovacuum_vacuum_cost_limit to
-1, the default, will have it use vacuum_cost_limit. However, it says
nothing about what setting it to 0 does. In the code, everywhere assumes
if autovacuum_vacuum_cost_limit is 0 OR -1, use vacuum_cost_limit.

This is in contrast to autovacuum_vacuum_cost_delay, for which 0 means
to disable it -- so setting autovacuum_vacuum_cost_delay to 0 will
specifically not fall back to vacuum_cost_limit.

I think the problem is that 0 is not a valid cost limit (i.e. it has no
meaning like infinity/no limit), so we basically don't want to allow the
cost limit to be set to 0, but GUC values have to be a range with a max
and a min, so we can't just exclude 0 if we want to allow -1 (as far as
I know). I think it would be nice to be able to specify multiple valid
ranges for GUCs to the GUC machinery.

So, to answer your question, yes, the code and docs are a bit
out-of-sync.

> idea and a patch in good condition close to being committable.

I will take a pass at splitting up the main commit into two. However, I
have attached a new version with the other specific updates discussed in
this thread. Feel free to provide review on this version in the meantime.

- Melanie
From c48b9cc5da87d980fff7a0131da72c28865ef310 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Thu, 23 Mar 2023 18:21:24 -0400
Subject: [PATCH v7 1/2] Make failsafe_active global

In preparation for future work to update the cost-based delay parameters
more frequently during vacuum, move the failsafe_active status into a
global variable which can be accessed from all parts of vacuum code. It
will be used in combination with VacuumCostDelay to keep
VacuumCostActive up-to-date during failsafe vacuuming..
---
 src/backend/access/heap/vacuumlazy.c  | 16 +++++++---------
 src/backend/commands/vacuum.c         |  4 ++++
 src/backend/commands/vacuumparallel.c |  1 +
 src/include/commands/vacuum.h         |  3 +++
 4 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 8f14cf85f3..f4755bcc4b 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -153,8 +153,6 @@ typedef struct LVRelState
 	bool		aggressive;
 	/* Use visibility map to skip? (disabled by DISABLE_PAGE_SKIPPING) */
 	bool		skipwithvm;
-	/* Wraparound failsafe has been triggered? */
-	bool		failsafe_active;
 	/* Consider index vacuuming bypass optimization? */
 	bool		consider_bypass_optimization;
 
@@ -391,7 +389,7 @@ heap_vacuum_rel(Relation rel, VacuumParams *params,
 	Assert(params->index_cleanup != VACOPTVALUE_UNSPECIFIED);
 	Assert(params->truncate != VACOPTVALUE_UNSPECIFIED &&
 		   params->truncate != VACOPTVALUE_AUTO);
-	vacrel->failsafe_active = false;
+	VacuumFailsafeActive = false;
 	vacrel->consider_bypass_optimization = true;
 	vacrel->do_index_vacuuming = true;
 	vacrel->do_index_cleanup = true;
@@ -709,7 +707,7 @@ heap_vacuum_rel(Relation rel, VacuumParams *params,
 			}
 			else
 			{
-				if (!vacrel->failsafe_active)
+				if (!VacuumFailsafeActive)
 					appendStringInfoString(&buf, _("index scan bypassed: "));
 				else
 					appendStringInfoString(&buf, _("index scan bypassed by failsafe: "));
@@ -2293,7 +2291,7 @@ lazy_vacuum(LVRelState *vacrel)
 		 * vacuuming or heap vacuuming.  This VACUUM operation won't end up
 		 * back here again.
 		 */
-		Assert(vacrel->failsafe_active);
+		Assert(VacuumFailsafeActive);
 	}
 
 	/*
@@ -2374,7 +2372,7 @@ lazy_vacuum_all_indexes(LVRelState *vacrel)
 	 */
 	Assert(vacrel->num_index_scans > 0 ||
 		   vacrel->dead_items->num_items == vacrel->lpdead_items);
-	Assert(allindexes || vacrel->failsafe_active);
+	Assert(allindexes || VacuumFailsafeActive);
 
 	/*
 	 * Increase and report the number of index scans.
@@ -2616,12 +2614,12 @@ static bool
 lazy_check_wraparound_failsafe(LVRelState *vacrel)
 {
 	/* Don't warn more than once per VACUUM */
-	if (vacrel->failsafe_active)
+	if (VacuumFailsafeActive)
 		return true;
 
 	if (unlikely(vacuum_xid_failsafe_check(&vacrel->cutoffs)))
 	{
-		vacrel->failsafe_active = true;
+		VacuumFailsafeActive = true;
 
 		/* Disable index vacuuming, index cleanup, and heap rel truncation */
 		vacrel->do_index_vacuuming = false;
@@ -2811,7 +2809,7 @@ should_attempt_truncation(LVRelState *vacrel)
 {
 	BlockNumber possibly_freeable;
 
-	if (!vacrel->do_rel_truncate || vacrel->failsafe_active ||
+	if (!vacrel->do_rel_truncate || VacuumFailsafeActive ||
 		old_snapshot_threshold >= 0)
 		return false;
 
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index c54360a6a0..2d5ea570a2 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -77,6 +77,7 @@ int			vacuum_multixact_failsafe_age;
 static MemoryContext vac_context = NULL;
 static BufferAccessStrategy vac_strategy;

 /*
  * Variables for cost-based parallel vacuum.  See comments atop
@@ -492,6 +493,7 @@ vacuum(List *relations, VacuumParams *params,
 
 		in_vacuum = true;
 		VacuumCostActive = (VacuumCostDelay > 0);
+		VacuumFailsafeActive = false;
 		VacuumCostBalance = 0;
 		VacuumPageHit = 0;
 		VacuumPageMiss = 0;
@@ -1850,6 +1852,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params, bool skip_privs)
 
 	Assert(params != NULL);

 
 	/* Set cost-based vacuum delay */
+	VacuumFailsafeActive = false;
 	VacuumCostActive = (VacuumCostDelay > 0);
 	VacuumCostBalance = 0;
 	VacuumPageHit = 0;
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index bdfd96cfec..a1bf3bfaa5 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -301,6 +301,9 @@ extern PGDLLIMPORT int vacuum_multixact_freeze_table_age;
 extern PGDLLIMPORT int vacuum_failsafe_age;
 extern PGDLLIMPORT int vacuum_multixact_failsafe_age;
 
+/* Indicates if wraparound failsafe has been triggered */
+extern bool VacuumFailsafeActive;
+
 /* Variables for cost-based parallel vacuum */
 extern PGDLLIMPORT pg_atomic_uint32 *VacuumSharedCostBalance;
 extern PGDLLIMPORT pg_atomic_uint32 *VacuumActiveNWorkers;
-- 
2.37.2

From 37b9db4404d51c7a4e695472f9ea9f96392f9dd1 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Thu, 23 Mar 2023 18:41:10 -0400
Subject: [PATCH v7 2/2] [auto]vacuum reloads config file more often

Reviewed-by: Masahiko Sawada <[email-masked]>
Reviewed-by: Daniel Gustafsson <[email-masked]>
Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_buP5wzsho3qNw5o9_R0pF69FRM5hgCmr-mvXmGXwdA7A%40mail.gmail.com#5e6771d4cdca4db6efc2acec2dce0bc7
---
 src/backend/commands/vacuum.c       |  52 +++++--
 src/backend/postmaster/autovacuum.c | 218 ++++++++++++----------------
 src/include/postmaster/autovacuum.h |   2 +
 3 files changed, 140 insertions(+), 132 deletions(-)

index 2d5ea570a2..c4c361ae94 100644
 
 bool		VacuumFailsafeActive = false;
 
@@ -315,8 +317,7 @@ vacuum(List *relations, VacuumParams *params,
 	static bool in_vacuum = false;
 
 	const char *stmttype;
 
 	Assert(params != NULL);
 
@@ -333,10 +334,10 @@ vacuum(List *relations, VacuumParams *params,
 
 	/*
@@ -458,7 +459,7 @@ vacuum(List *relations, VacuumParams *params,
@@ -476,7 +477,7 @@ vacuum(List *relations, VacuumParams *params,
 
 		/* ActiveSnapshot is not set by autovacuum */
 		if (ActiveSnapshotSet())
@@ -529,7 +530,7 @@ vacuum(List *relations, VacuumParams *params,
 				}

 				if (use_own_xacts)
 				{
@@ -552,6 +553,7 @@ vacuum(List *relations, VacuumParams *params,
 	{
 		in_vacuum = false;
 		VacuumCostActive = false;
+		analyze_in_outer_xact = false;
 	}
 	PG_END_TRY();
 
@@ -2219,9 +2221,33 @@ vacuum_delay_point(void)
 	/* Always check for interrupts */
 	CHECK_FOR_INTERRUPTS();
 
-	if (!VacuumCostActive || InterruptPending)
+	if (InterruptPending ||
+		(!VacuumCostActive && !ConfigReloadPending))
 		return;
 
+	/*
@@ -2252,8 +2278,14 @@ vacuum_delay_point(void)
 
 		VacuumCostBalance = 0;

index c0e2e00a7e..ad872d8c73 100644

 
 typedef struct WorkerInfoData *WorkerInfo;
@@ -273,6 +273,8 @@ typedef struct AutoVacuumWorkItem
  * av_startingWorker pointer to WorkerInfo currently being started (cleared by
  *					the worker itself as soon as it's up and running)
  * av_workItems		work item array
+ * av_nworkers_for_balance the number of autovacuum workers to use when
+ * 					calculating the per worker cost limit
  *
  * This struct is protected by AutovacuumLock, except for av_signal and parts
  * of the worker list (see above).
@@ -286,6 +288,7 @@ typedef struct
 	dlist_head	av_runningWorkers;
 	WorkerInfo	av_startingWorker;
 	AutoVacuumWorkItem av_workItems[NUM_WORKITEMS];
+	pg_atomic_uint32 av_nworkers_for_balance;
 } AutoVacuumShmemStruct;
 
 static AutoVacuumShmemStruct *AutoVacuumShmem;
@@ -820,7 +823,7 @@ HandleAutoVacLauncherInterrupts(void)
 			AutoVacLauncherShutdown();

@@ -1756,9 +1759,6 @@ FreeWorkerInfo(int code, Datum arg)
@@ -1774,99 +1774,97 @@ FreeWorkerInfo(int code, Datum arg)
 }
 
 /*
+	else if (autovacuum_vac_cost_delay >= 0)
+		VacuumCostDelay = autovacuum_vac_cost_delay;
+ * call this regularly in case av_nworkers_for_balance has been updated by
+ * another worker or by the autovacuum launcher. They also must call this after
+ * every config reload, in case VacuumCostLimit was overwritten.
+		pg_atomic_read_u32(&AutoVacuumShmem->av_nworkers_for_balance);
 
 /*
  * autovac_balance_cost

 
-	/*
+		pg_atomic_read_u32(&AutoVacuumShmem->av_nworkers_for_balance);

+		pg_atomic_write_u32(&AutoVacuumShmem->av_nworkers_for_balance,
+							nworkers_for_balance);
 }
 
 /*
@@ -2312,14 +2310,15 @@ do_autovacuum(void)
 		autovac_table *tab;
 		bool		isshared;
 		bool		skipit;
-		double		stdVacuumCostDelay;
-		int			stdVacuumCostLimit;
 		dlist_iter	iter;
 
 		CHECK_FOR_INTERRUPTS();
 
 		/*
@@ -2416,32 +2415,18 @@ do_autovacuum(void)
 			continue;
 		}
 
-		/*

 
-		/* done */
 
@@ -2525,19 +2510,15 @@ deleted:
 
 		/*
 		 * Remove my info from shared memory.  We could, but intentionally
-		 * don't, clear wi_cost_limit and friends --- this is on the
-		 * assumption that we probably have more to do with similar cost
-		 * settings, so we don't want to give up our share of I/O for a very
-		 * short interval and thereby thrash the global balance.
+		 * don't, set wi_dobalance to false on the assumption that we are more
+		 * likely than not to vacuum a table with no table options next, so we
+		 * don't want to give up our share of I/O for a very short interval
+		 * and thereby thrash the global balance.
 		 */
 		LWLockAcquire(AutovacuumScheduleLock, LW_EXCLUSIVE);
 		MyWorkerInfo->wi_tableoid = InvalidOid;
 		MyWorkerInfo->wi_sharedrel = false;
 		LWLockRelease(AutovacuumScheduleLock);
-
-		/* restore vacuum cost GUCs for the next iteration */
-		VacuumCostDelay = stdVacuumCostDelay;
-		VacuumCostLimit = stdVacuumCostLimit;
 	}
 
 	/*
@@ -2569,6 +2550,8 @@ deleted:
 		{
 
 		LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
@@ -2801,8 +2784,6 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
 		int			freeze_table_age;
 		int			multixact_freeze_min_age;
 		int			multixact_freeze_table_age;
-		int			vac_cost_limit;
-		double		vac_cost_delay;
 		int			log_min_duration;
 
 		/*
@@ -2812,20 +2793,6 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
 		 * defaults, autovacuum's own first and plain vacuum second.
 		 */
 
@@ -2881,8 +2848,10 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
@@ -3374,10 +3343,15 @@ AutoVacuumShmemInit(void)
 
+
+		pg_atomic_init_u32(&AutoVacuumShmem->av_nworkers_for_balance, 1);

---

From: Masahiko Sawada <[email-masked]>
Date: Fri, 24 Mar 2023 14:21:03 +0900

> of knowing if it had incremented or decremented the number, so we can't

What kind of crash are you concerned about? If a worker raises an
ERROR, we can catch it in PG_CATCH() block. If it's a FATAL, we can do
that in FreeWorkerInfo(). A PANIC error ends up crashing the entire
server.

                 in_vacuum = true;

I think we need to reset VacuumFailsafeActive also in PG_FINALLY()
block in vacuum().

One comment on 0002 patch:

+        /*

I think we need comments on why we don't reload the config file if
we're analyzing a table in a user transaction.

> > idea and a patch in good condition close to being committable.

Another approach would be to make VacuumCostActive a ternary value:
on, off, and never. When we trigger the failsafe mode we switch it to
never, meaning that it never becomes active even after reloading the
config file. A good point is that we don't need to add a new global
variable, but I'm not sure it's better than the current approach.

Regards,

---

From: Melanie Plageman <[email-masked]>
Date: Fri, 24 Mar 2023 13:27:45 -0400

> that in FreeWorkerInfo(). A PANIC error ends up crashing the entire

Yes, but what about a worker that segfaults? Since table AMs can define
relation_vacuum(), this seems like a real possibility.

I'll address your other code feedback in the next version.

I realized nworkers_for_balance should be initialized to 0 and not 1 --
1 is misleading since there are often 0 autovac workers. We just never
want to use nworkers_for_balance when it is 0. But, workers put a floor
of 1 on the number when they divide limit/nworkers_for_balance (since
they know there must be at least one worker right now since they are a
worker). I thought about whether or not they should call
autovac_balance_cost() if they find that nworkers_for_balance is 0 when
updating their own limit, but I'm not sure.

> variable, but I'm not sure it's better than the current approach.

Hmm, this is interesting. I don't love the word "never" since it kind of
implies a duration longer than the current table being vacuumed. But we
could find a different word or just document it well. For clarity, we
might want to call it failsafe_mode or something.

I wonder if the primary drawback to converting
LVRelState->failsafe_active to a global VacuumFailsafeActive is just the
general rule of limiting scope to the minimum needed.

- Melanie

---

From: Melanie Plageman <[email-masked]>
Date: Sat, 25 Mar 2023 15:03:56 -0400

> file, though (with autovac not benefitting in the first commit).

So, I realized we could actually do as you say and have autovac workers
update their wi_cost_delay and keep the balance changes in a separate
commit. I've done this in attached v8.

Workers take the exclusive lock to update their wi_cost_delay and
wi_cost_limit only when there is a config reload. So, there is one
commit that implements this behavior and a separate commit to revise the
worker rebalancing.

Note that we must have the workers also update wi_cost_limit_base and
then call autovac_balance_cost() when they reload the config file
(instead of waiting for launcher to call autovac_balance_cost()) to
avoid potentially calculating the sleep with a new value of cost delay
and an old value of cost limit.

In the commit which revises the worker rebalancing, I'm still wondering
if wi_dobalance should be an atomic flag -- probably not worth it,
right?

> updating their own limit, but I'm not sure.

I've gone ahead and updated this. I haven't made the workers call
autovac_balance_cost() if they find that nworkers_for_balance is 0 when
they try and use it when updating their limit because I'm not sure if
this can happen. I would be interested in input here.

I'm also still interested in feedback on the variable name
av_nworkers_for_balance.

> general rule of limiting scope to the minimum needed.

Okay, so I've changed my mind about this. I like having a ternary for
VacuumCostActive and keeping failsafe_active in LVRelState. What I
didn't like was having non-vacuum code have to care about the
distinction between failsafe + inactive and just inactive. To handle
this, I converted VacuumCostActive to VacuumCostInactive since there are
two inactive cases (inactive and failsafe and plain inactive) and only
one active case. Then, I defined VacuumCostInactive as an int but use
enum values for it in vacuum code to distinguish between failsafe +
inactive and just inactive (I call it VACUUM_COST_INACTIVE_AND_LOCKED
and VACUUM_COST_INACTIVE_AND_UNLOCKED). Non-vacuum code only needs to
check if VacuumCostInactive is 0 like if (!VacuumCostInactive). I'm
happy with the result, and I think it employs only well-defined C
behavior.

- Melanie
From 8b9fcf7c10353dcacb4ac16515aad0ce34565566 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Fri, 24 Mar 2023 13:38:20 -0400
Subject: [PATCH v8 3/4] [auto]vacuum reloads config file more often

Reviewed-by: Masahiko Sawada <[email-masked]>
Reviewed-by: Daniel Gustafsson <[email-masked]>
Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_buP5wzsho3qNw5o9_R0pF69FRM5hgCmr-mvXmGXwdA7A%40mail.gmail.com#5e6771d4cdca4db6efc2acec2dce0bc7
---
 src/backend/commands/vacuum.c       | 72 +++++++++++++++++++++++++----
 src/backend/postmaster/autovacuum.c | 30 +++++++++++-
 src/include/postmaster/autovacuum.h |  3 +-
 3 files changed, 92 insertions(+), 13 deletions(-)

index eb126f2247..cb32078c19 100644

 /*
 
 	const char *stmttype;
 
 	Assert(params != NULL);

 	/*
 
 		/* ActiveSnapshot is not set by autovacuum */
 		if (ActiveSnapshotSet())
@@ -544,7 +545,7 @@ vacuum(List *relations, VacuumParams *params,
 				}

 				if (use_own_xacts)
 				{
@@ -568,6 +569,7 @@ vacuum(List *relations, VacuumParams *params,
 		in_vacuum = false;
 		VacuumCostInactive = VACUUM_COST_INACTIVE_AND_UNLOCKED;
 		VacuumCostBalance = 0;
+		analyze_in_outer_xact = false;
 	}
 	PG_END_TRY();
 
@@ -2233,7 +2235,52 @@ vacuum_delay_point(void)
 	/* Always check for interrupts */
 	CHECK_FOR_INTERRUPTS();
 
-	if (VacuumCostInactive || InterruptPending)
+	if (InterruptPending ||
+		(VacuumCostInactive && !ConfigReloadPending))
+		return;
+
+	/*
+	 * Reload the configuration file if requested. This allows changes to
+	 * [autovacuum_]vacuum_cost_limit and [autovacuum_]vacuum_cost_delay to
+	 * take effect while a table is being vacuumed or analyzed. Analyze should
+	 * not reload configuration file if it is in an outer transaction, as GUC
+	 * values shouldn't be allowed to refer to some uncommitted state (e.g.
+	 * database objects created in this transaction).
+	 */
+	if (ConfigReloadPending && !analyze_in_outer_xact)
+	{
+		ConfigReloadPending = false;
+		ProcessConfigFile(PGC_SIGHUP);
+
+		/*
+		 * Autovacuum workers must restore the correct values of
+		 * VacuumCostLimit and VacuumCostDelay in case they were overwritten
+		 * by reload.
+		 */
+		AutoVacuumUpdateCosts();
+		AutoVacuumOverrideCosts();
+
+		/*
+		 * If configuration changes are allowed to impact VacuumCostInactive,
+		 * make sure it is updated.
+		 */
+		if (VacuumCostInactive == VACUUM_COST_INACTIVE_AND_LOCKED)
+			return;
+
+		if (VacuumCostDelay > 0)
+			VacuumCostInactive = VACUUM_COST_ACTIVE;
+		else
+		{
+			VacuumCostInactive = VACUUM_COST_INACTIVE_AND_UNLOCKED;
+			VacuumCostBalance = 0;
+		}
+	}
+
+	/*
+	 * If we disabled cost-based delays after reloading the config file,
+	 * return.
+	 */
+	if (VacuumCostInactive)
 		return;
 
 	/*
@@ -2266,8 +2313,13 @@ vacuum_delay_point(void)
 
 		VacuumCostBalance = 0;
 
-		/* update balance values for workers */
-		AutoVacuumUpdateDelay();
+		/*
+		 * For autovacuum workers, someone may have called
+		 * autovac_balance_cost() since they last updated their
+		 * VacuumCostLimit above. Do so again now to ensure they have a
+		 * current value.
+		 */
+		AutoVacuumOverrideCosts();
 
 		/* Might have gotten an interrupt while sleeping */
 		CHECK_FOR_INTERRUPTS();
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index c0e2e00a7e..8ac14a44c8 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -1778,7 +1778,7 @@ FreeWorkerInfo(int code, Datum arg)
  * each a fraction of the total available I/O.
  */
 void
-AutoVacuumUpdateDelay(void)
+AutoVacuumOverrideCosts(void)
 {
 	if (MyWorkerInfo)
 	{
@@ -1787,6 +1787,29 @@ AutoVacuumUpdateDelay(void)
 	}
 }
 
+/*
+ * Caller must not already hold the AutovacuumLock
+ */
+void
+AutoVacuumUpdateCosts(void)
+{
+	/*
+	 * Even though this autovacuum worker may be vacuuming a table with a cost
+	 * limit table option and not a cost delay table option, we still don't
+	 * refresh the cost delay value.
+	 */
+	if (!MyWorkerInfo || !MyWorkerInfo->wi_dobalance)
+		return;
+
+	LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
+	MyWorkerInfo->wi_cost_delay = autovacuum_vac_cost_delay >= 0 ?
+		autovacuum_vac_cost_delay : VacuumCostDelay;
+	MyWorkerInfo->wi_cost_limit_base = autovacuum_vac_cost_limit > 0 ?
+		autovacuum_vac_cost_limit : VacuumCostLimit;
+	autovac_balance_cost();
+	LWLockRelease(AutovacuumLock);
+}
+
 /*
  * autovac_balance_cost
  *		Recalculate the cost limit setting for each active worker.
@@ -2320,6 +2343,9 @@ do_autovacuum(void)
 
 		/*
@@ -2437,7 +2463,7 @@ do_autovacuum(void)
 		autovac_balance_cost();
 
 		/* set the active cost parameters from the result of that */
-		AutoVacuumUpdateDelay();
+		AutoVacuumOverrideCosts();
 
 		/* done */
 		LWLockRelease(AutovacuumLock);
diff --git a/src/include/postmaster/autovacuum.h b/src/include/postmaster/autovacuum.h
index c140371b51..ee48e7123d 100644
--- a/src/include/postmaster/autovacuum.h
+++ b/src/include/postmaster/autovacuum.h
@@ -64,7 +64,8 @@ extern int	StartAutoVacWorker(void);
 extern void AutoVacWorkerFailed(void);
 
 /* autovacuum cost-delay balancer */
-extern void AutoVacuumUpdateDelay(void);
+extern void AutoVacuumOverrideCosts(void);
+extern void AutoVacuumUpdateCosts(void);
 
 #ifdef EXEC_BACKEND

From 9ade10882c6b2daafb846be667de04225046e157 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Sat, 25 Mar 2023 11:59:33 -0400
Subject: [PATCH v8 1/4] Zero out VacuumCostBalance

Though it is unlikely to matter, we should zero out VacuumCostBalance
whenever we may be transitioning the state of VacuumCostActive to false.
---
 src/backend/commands/vacuum.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index c54360a6a0..7d01df7e48 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -550,6 +550,7 @@ vacuum(List *relations, VacuumParams *params,
 	{
 		in_vacuum = false;
 		VacuumCostActive = false;
+		VacuumCostBalance = 0;
 	}
 	PG_END_TRY();
 
-- 
2.37.2

From 0a546a538b75ee1a736ff9a09a31412c0b323082 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Sat, 25 Mar 2023 14:14:55 -0400
Subject: [PATCH v8 4/4] Improve autovacuum worker cost balancing

Before the prior commit, an autovacuum worker's wi_cost_limit was set
only at the beginning of vacuuming a table, after reloading the config
file. Therefore, at the time that autovac_balance_cost() is called,
workers vacuuming tables with no table options could still have
different values for their wi_cost_limit_base and wi_cost_delay.

Now that the cost parameters can be updated while vacuuming a table,
workers will (within some margin of error) have no reason to have
different values for cost limit and cost delay (in the absence of table
options). This removes the rationale for keeping cost limit and cost
delay in shared memory. Balancing the cost limit requires only the
number of active autovacuum workers vacuuming a table with no cost-based
table options.
---
 src/backend/commands/vacuum.c       |  18 ++-
 src/backend/postmaster/autovacuum.c | 237 ++++++++++++----------------
 src/include/postmaster/autovacuum.h |   4 +-
 3 files changed, 112 insertions(+), 147 deletions(-)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index cb32078c19..54ad76a729 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -2257,12 +2257,13 @@ vacuum_delay_point(void)
 		 * VacuumCostLimit and VacuumCostDelay in case they were overwritten
 		 * by reload.
 		 */
-		AutoVacuumUpdateCosts();
-		AutoVacuumOverrideCosts();
+		AutoVacuumUpdateDelay();
+		AutoVacuumUpdateLimit();
 
 		/*
 		 * If configuration changes are allowed to impact VacuumCostInactive,
-		 * make sure it is updated.
+		 * make sure it is updated. Autovacuum workers will have already done
+		 * this in AutoVacuumUpdateDelay()
 		 */
 		if (VacuumCostInactive == VACUUM_COST_INACTIVE_AND_LOCKED)
 			return;
@@ -2314,12 +2315,13 @@ vacuum_delay_point(void)
 		VacuumCostBalance = 0;
 
 		/*
-		 * For autovacuum workers, someone may have called
-		 * autovac_balance_cost() since they last updated their
-		 * VacuumCostLimit above. Do so again now to ensure they have a
-		 * current value.
+		 * Update limit values for autovacuum workers. We must always do this
+		 * in case the autovacuum launcher or another autovacuum worker has
+		 * recalculated the number of workers across which we must balance the
+		 * limit. This is done by the launcher when launching a new worker and
+		 * by workers before vacuuming each table.
 		 */
-		AutoVacuumOverrideCosts();
+		AutoVacuumUpdateLimit();
 
index 8ac14a44c8..0c20442fb1 100644

 

 
@@ -1773,123 +1773,114 @@ FreeWorkerInfo(int code, Datum arg)
 	}
 }
 
+
 /*
- * Update the cost-based delay parameters, so that multiple workers consume
- * each a fraction of the total available I/O.
+ * Update VacuumCostDelay with the correct value for an autovacuum worker,
+ * given the value of other relevant cost-based delay parameters. Autovacuum
+ * workers should call this after every config reload, in case VacuumCostDelay
+ * was overwritten.
  */
 void
-AutoVacuumOverrideCosts(void)
+AutoVacuumUpdateDelay(void)
 {
-	if (MyWorkerInfo)
+	if (!am_autovacuum_worker)
+		return;
+
+	if (av_table_option_cost_delay >= 0)
+		VacuumCostDelay = av_table_option_cost_delay;
+	else if (autovacuum_vac_cost_delay >= 0)
+		VacuumCostDelay = autovacuum_vac_cost_delay;
+
+	/*
+	 * If configuration changes are allowed to impact VacuumCostInactive, make
+	 * sure it is updated.
+	 */
+	if (VacuumCostInactive == VACUUM_COST_INACTIVE_AND_LOCKED)
+		return;
+
+	if (VacuumCostDelay > 0)
+		VacuumCostInactive = VACUUM_COST_ACTIVE;
+	else
 	{
-		VacuumCostDelay = MyWorkerInfo->wi_cost_delay;
-		VacuumCostLimit = MyWorkerInfo->wi_cost_limit;
+		VacuumCostInactive = VACUUM_COST_INACTIVE_AND_UNLOCKED;
+		VacuumCostBalance = 0;
 	}
 }
 
+
 /*
- * Caller must not already hold the AutovacuumLock
+ * Update VacuumCostLimit with the correct value for an autovacuum worker,
+ * given the value of other relevant cost limit parameters and the number of
+ * workers across which the limit must be balanced. Autovacuum workers must
+ * call this regularly in case av_nworkers_for_balance has been updated by
+ * another worker or by the autovacuum launcher. They also must call this after
+ * every config reload, in case VacuumCostLimit was overwritten.
  */
 void
-AutoVacuumUpdateCosts(void)
+AutoVacuumUpdateLimit(void)
 {
+	if (!am_autovacuum_worker)
+		return;
+
 	/*
-	 * Even though this autovacuum worker may be vacuuming a table with a cost
-	 * limit table option and not a cost delay table option, we still don't
-	 * refresh the cost delay value.
+	 * note: in cost_limit, zero also means use value from elsewhere, because
+	 * zero is not a valid value.
 	 */
-	if (!MyWorkerInfo || !MyWorkerInfo->wi_dobalance)
-		return;
+	if (av_table_option_cost_limit > 0)
+		VacuumCostLimit = av_table_option_cost_limit;
+	else
+	{
+		/* There is at least 1 autovac worker (this worker). */
+		int			nworkers_for_balance = Max(pg_atomic_read_u32(
+					&AutoVacuumShmem->av_nworkers_for_balance), 1);
 
-	LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
-	MyWorkerInfo->wi_cost_delay = autovacuum_vac_cost_delay >= 0 ?
-		autovacuum_vac_cost_delay : VacuumCostDelay;
-	MyWorkerInfo->wi_cost_limit_base = autovacuum_vac_cost_limit > 0 ?
+		int			vac_cost_limit = autovacuum_vac_cost_limit > 0 ?
 		autovacuum_vac_cost_limit : VacuumCostLimit;
-	autovac_balance_cost();
-	LWLockRelease(AutovacuumLock);
+
+		int			balanced_cost_limit = vac_cost_limit / nworkers_for_balance;
+
+		VacuumCostLimit = Max(Min(balanced_cost_limit, vac_cost_limit), 1);
+	}
 }
 
+
 /*
  * autovac_balance_cost

 
-	/*

 
 /*
@@ -2335,8 +2326,6 @@ do_autovacuum(void)
 		autovac_table *tab;
 		bool		isshared;
 		bool		skipit;
-		double		stdVacuumCostDelay;
-		int			stdVacuumCostLimit;
 		dlist_iter	iter;
 
 		CHECK_FOR_INTERRUPTS();
@@ -2442,32 +2431,18 @@ do_autovacuum(void)
 			continue;
 		}
 
-		/*
 
-		AutoVacuumOverrideCosts();

@@ -2551,19 +2526,15 @@ deleted:
 
 		/*
 
 	/*
@@ -2595,6 +2566,8 @@ deleted:
 		{
 
 		LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
@@ -2827,8 +2800,6 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
 		int			freeze_table_age;
 		int			multixact_freeze_min_age;
 		int			multixact_freeze_table_age;
-		int			vac_cost_limit;
-		double		vac_cost_delay;
 		int			log_min_duration;
 
 		/*
@@ -2838,20 +2809,6 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
 		 * defaults, autovacuum's own first and plain vacuum second.
 		 */
 
@@ -2907,8 +2864,10 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
@@ -3400,10 +3359,14 @@ AutoVacuumShmemInit(void)
 
+
 		/* initialize the WorkerInfo free list */
 		for (i = 0; i < autovacuum_max_workers; i++)
 			dlist_push_head(&AutoVacuumShmem->av_freeWorkers,
 							&worker[i].wi_links);
+
+		pg_atomic_init_u32(&AutoVacuumShmem->av_nworkers_for_balance, 0);
+
 	}
 	else
 		Assert(found);
diff --git a/src/include/postmaster/autovacuum.h b/src/include/postmaster/autovacuum.h
index ee48e7123d..7b462866c9 100644
--- a/src/include/postmaster/autovacuum.h
+++ b/src/include/postmaster/autovacuum.h
@@ -64,8 +64,8 @@ extern int	StartAutoVacWorker(void);
 extern void AutoVacWorkerFailed(void);
 
 /* autovacuum cost-delay balancer */
-extern void AutoVacuumOverrideCosts(void);
-extern void AutoVacuumUpdateCosts(void);
+extern void AutoVacuumUpdateDelay(void);
+extern void AutoVacuumUpdateLimit(void);
 
 #ifdef EXEC_BACKEND

From 3a9492b1f3eaacafc730aea0d53085046886ecad Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Sat, 25 Mar 2023 12:05:18 -0400
Subject: [PATCH v8 2/4] Make VacuumCostActive failsafe-aware

While vacuuming a table in failsafe mode, VacuumCostActive should not be
re-enabled. This currently isn't a problem because vacuum cost
parameters are only refreshed in between vacuuming tables and failsafe
status is reset for every table. In preparation for allowing vacuum cost
parameters to be updated more frequently, make vacuum cost status more
expressive.

VacuumCostActive is now VacuumCostInactive, as it can only be active in
one way but it can be inactive in two ways. If performing a failsafe
vacuum, the vacuum cost status cannot be enabled and is effectively
"locked". If performing a non-failsafe vacuum, the vacuum cost status
may be active or inactive. To express this, VacuumCostInactive can be
one of three statuses: VACUUM_COST_INACTIVE_AND_LOCKED,
VACUUM_COST_ACTIVE_AND_LOCKED, and VACUUM_COST_ACTIVE.

VacuumCostInactive is defined as an integer because we do not want
non-vacuum code concerning itself with the distinction between the three
statuses -- only with whether or not VacuumCostInactive == 0 or not.
---
 src/backend/access/heap/vacuumlazy.c  |  2 +-
 src/backend/commands/vacuum.c         | 23 ++++++++++++++++++++---
 src/backend/commands/vacuumparallel.c |  8 ++++++--
 src/backend/storage/buffer/bufmgr.c   |  8 ++++----
 src/backend/utils/init/globals.c      |  2 +-
 src/include/commands/vacuum.h         |  8 ++++++++
 src/include/miscadmin.h               |  3 +--
 7 files changed, 41 insertions(+), 13 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 8f14cf85f3..040a4e931b 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -2637,7 +2637,7 @@ lazy_check_wraparound_failsafe(LVRelState *vacrel)
 						 "You might also need to consider other ways for VACUUM to keep up with the allocation of transaction IDs.")));
 
 		/* Stop applying cost limits from this point on */
-		VacuumCostActive = false;
+		VacuumCostInactive = VACUUM_COST_INACTIVE_AND_LOCKED;
 		VacuumCostBalance = 0;
 
 		return true;
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 7d01df7e48..eb126f2247 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -491,7 +491,6 @@ vacuum(List *relations, VacuumParams *params,
 		ListCell   *cur;
 
 		in_vacuum = true;
-		VacuumCostActive = (VacuumCostDelay > 0);
 		VacuumCostBalance = 0;
 		VacuumPageHit = 0;
 		VacuumPageMiss = 0;
@@ -507,6 +506,24 @@ vacuum(List *relations, VacuumParams *params,
 		{
 			VacuumRelation *vrel = lfirst_node(VacuumRelation, cur);
 
+			/*
+			 * failsafe_active is reset per relation, so we must be sure that
+			 * VacuumCostInactive is set to either VACUUM_COST_INACTIVE or
+			 * VACUUM_COST_INACTIVE_AND_UNLOCKED in between vacuuming
+			 * relations.
+			 */
+			VacuumCostInactive = VacuumCostDelay > 0 ? VACUUM_COST_ACTIVE :
+				VACUUM_COST_INACTIVE_AND_UNLOCKED;
+
+			/*
+			 * We should not have transitioned VacuumCostInactive from
+			 * VACUUM_COST_ACTIVE to VACUUM_COST_INACTIVE_AND_UNLOCKED above,
+			 * as that should have happened when we changed the value of
+			 * VacuumCostDelay.
+			 */
+			Assert(VacuumCostInactive == VACUUM_COST_ACTIVE ||
+				   VacuumCostBalance == 0);
+
 			if (params->options & VACOPT_VACUUM)
 			{
 				if (!vacuum_rel(vrel->oid, vrel->relation, params, false))
@@ -549,7 +566,7 @@ vacuum(List *relations, VacuumParams *params,
 	PG_FINALLY();
 	{
 		in_vacuum = false;
-		VacuumCostActive = false;
+		VacuumCostInactive = VACUUM_COST_INACTIVE_AND_UNLOCKED;
 		VacuumCostBalance = 0;
 	}
 	PG_END_TRY();
@@ -2216,7 +2233,7 @@ vacuum_delay_point(void)
 	/* Always check for interrupts */
 	CHECK_FOR_INTERRUPTS();
 
-	if (!VacuumCostActive || InterruptPending)
+	if (VacuumCostInactive || InterruptPending)
 		return;
 
 	/*
diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c
index bcd40c80a1..266bf6bb4c 100644
--- a/src/backend/commands/vacuumparallel.c
+++ b/src/backend/commands/vacuumparallel.c
@@ -989,8 +989,12 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
 												 PARALLEL_VACUUM_KEY_DEAD_ITEMS,
 												 false);
 
-	/* Set cost-based vacuum delay */
-	VacuumCostActive = (VacuumCostDelay > 0);
+	/*
+	 * Set cost-based vacuum delay Parallel vacuum workers will not execute
+	 * failsafe VACUUM.
+	 */
+	VacuumCostInactive = VacuumCostDelay > 0 ? VACUUM_COST_ACTIVE :
+		VACUUM_COST_INACTIVE_AND_UNLOCKED;
 	VacuumCostBalance = 0;
 	VacuumPageHit = 0;
 	VacuumPageMiss = 0;
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 95212a3941..6d3dd26fc7 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -893,7 +893,7 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
 			*hit = true;
 			VacuumPageHit++;
 
-			if (VacuumCostActive)
+			if (!VacuumCostInactive)
 				VacuumCostBalance += VacuumCostPageHit;
 
 			TRACE_POSTGRESQL_BUFFER_READ_DONE(forkNum, blockNum,
@@ -1098,7 +1098,7 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
 	}
 
 	VacuumPageMiss++;
-	if (VacuumCostActive)
+	if (!VacuumCostInactive)
 		VacuumCostBalance += VacuumCostPageMiss;
 
 	TRACE_POSTGRESQL_BUFFER_READ_DONE(forkNum, blockNum,
@@ -1672,7 +1672,7 @@ MarkBufferDirty(Buffer buffer)
 	{
 		VacuumPageDirty++;
 		pgBufferUsage.shared_blks_dirtied++;
-		if (VacuumCostActive)
+		if (!VacuumCostInactive)
 			VacuumCostBalance += VacuumCostPageDirty;
 	}
 }
@@ -4199,7 +4199,7 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
 		{
 			VacuumPageDirty++;
 			pgBufferUsage.shared_blks_dirtied++;
-			if (VacuumCostActive)
+			if (!VacuumCostInactive)
 				VacuumCostBalance += VacuumCostPageDirty;
 		}
 	}
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 1b1d814254..608ebb9182 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -150,4 +150,4 @@ int64		VacuumPageMiss = 0;
 int64		VacuumPageDirty = 0;
 
 int			VacuumCostBalance = 0;	/* working state for vacuum */
-bool		VacuumCostActive = false;
+int			VacuumCostInactive = 1;
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index bdfd96cfec..5c3e250b06 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -302,6 +302,14 @@ extern PGDLLIMPORT int vacuum_failsafe_age;
 extern PGDLLIMPORT int vacuum_multixact_failsafe_age;
 
 /* Variables for cost-based parallel vacuum */
+
+typedef enum VacuumCostStatus
+{
+	VACUUM_COST_INACTIVE_AND_LOCKED = -1,
+	VACUUM_COST_ACTIVE = 0,
+	VACUUM_COST_INACTIVE_AND_UNLOCKED = 1,
+}			VacuumCostStatus;
+
 extern PGDLLIMPORT pg_atomic_uint32 *VacuumSharedCostBalance;
 extern PGDLLIMPORT pg_atomic_uint32 *VacuumActiveNWorkers;
 extern PGDLLIMPORT int VacuumCostBalanceLocal;
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 06a86f9ac1..33e22733ae 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -274,8 +274,7 @@ extern PGDLLIMPORT int64 VacuumPageMiss;
 extern PGDLLIMPORT int64 VacuumPageDirty;
 
 extern PGDLLIMPORT int VacuumCostBalance;
-extern PGDLLIMPORT bool VacuumCostActive;
-
+extern PGDLLIMPORT int VacuumCostInactive;
 
 /* in tcop/postgres.c */
 
-- 
2.37.2

---

From: Melanie Plageman <[email-masked]>
Date: Mon, 27 Mar 2023 14:12:03 -0400

> commit that implements this behavior and a separate commit to revise the

So, I've attached an alternate version of the patchset which takes the
approach of having one commit which only enables cost-based delay GUC
refresh for VACUUM and another commit which enables it for autovacuum
and makes the changes to balancing variables.

I still think the commit which has workers updating their own
wi_cost_delay in vacuum_delay_point() is a bit weird. It relies on no one
else emulating our bad behavior and reading from wi_cost_delay without a
lock and on no one else deciding to ever write to wi_cost_delay (even
though it is in shared memory [this is the same as master]). It is only
safe because our process is the only one (right now) writing to
wi_cost_delay, so when we read from it without a lock, we know it isn't
being written to. And everyone else takes a lock when reading from
wi_cost_delay right now. So, it seems...not great.

This approach also introduces a function that is only around for
one commit until the next commit obsoletes it, which seems a bit silly.

Basically, I think it is probably better to just have one commit
enabling guc refresh for VACUUM and then another which correctly
implements what is needed for autovacuum to do the same.
Attached v9 does this.

I've provided both complete versions of both approaches (v9 and v8).

- Melanie
From 94c08c1b764619ad6cc3a0c75295f416e1863b26 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Sat, 25 Mar 2023 11:59:33 -0400
Subject: [PATCH v9 1/4] Zero out VacuumCostBalance

 
-- 
2.37.2

From 3bdf9414c5840bcc94a9c0292f25ec41d2e95b69 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Sat, 25 Mar 2023 14:14:55 -0400
Subject: [PATCH v9 4/4] Autovacuum refreshes cost-based delay params more
 often

The previous commit allowed VACUUM to reload the config file more often
so that cost-based delay parameters could take effect while VACUUMing a
relation. Autovacuum, however did not benefit from this change.

In order for autovacuum workers to safely update their own cost delay
and cost limit parameters without impacting performance, we had to
rethink when and how these values were accessed.

Previously, an autovacuum worker's wi_cost_limit was set only at the
beginning of vacuuming a table, after reloading the config file.
Therefore, at the time that autovac_balance_cost() is called, workers
vacuuming tables with no table options could still have different values
for their wi_cost_limit_base and wi_cost_delay.

Now that the cost parameters can be updated while vacuuming a table,
workers will (within some margin of error) have no reason to have
different values for cost limit and cost delay (in the absence of table
options). This removes the rationale for keeping cost limit and cost
delay in shared memory. Balancing the cost limit requires only the
number of active autovacuum workers vacuuming a table with no cost-based
table options.
---
 src/backend/commands/vacuum.c       |  24 ++-
 src/backend/postmaster/autovacuum.c | 228 +++++++++++++---------------
 src/include/postmaster/autovacuum.h |   1 +
 3 files changed, 124 insertions(+), 129 deletions(-)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 0e686c94b2..8e39a13285 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -2241,11 +2241,11 @@ vacuum_delay_point(void)
 
 	/*
 	 * Reload the configuration file if requested. This allows changes to
-	 * vacuum_cost_limit and vacuum_cost_delay to take effect while a table is
-	 * being vacuumed or analyzed. Analyze should not reload configuration
-	 * file if it is in an outer transaction, as GUC values shouldn't be
-	 * allowed to refer to some uncommitted state (e.g. database objects
-	 * created in this transaction).
+	 * [autovacuum]_vacuum_cost_limit and [autovacuum]_vacuum_cost_delay to
+	 * take effect while a table is being vacuumed or analyzed. Analyze should
+	 * not reload configuration file if it is in an outer transaction, as GUC
+	 * values shouldn't be allowed to refer to some uncommitted state (e.g.
+	 * database objects created in this transaction).
 	 */
 	if (ConfigReloadPending && !analyze_in_outer_xact)
 	{
@@ -2258,10 +2258,12 @@ vacuum_delay_point(void)
 		 * by reload.
 		 */
 		AutoVacuumUpdateDelay();
+		AutoVacuumUpdateLimit();
 
 		/*
@@ -2312,8 +2314,14 @@ vacuum_delay_point(void)
 
 		VacuumCostBalance = 0;

index 585d28148c..6fe16aca3a 100644

 

 
@@ -1773,100 +1773,114 @@ FreeWorkerInfo(int code, Datum arg)
 	}
 }
 
+
 /*
 
+
 /*
- * autovac_balance_cost
-static void
-autovac_balance_cost(void)
 	 * note: in cost_limit, zero also means use value from elsewhere, because
 	 * zero is not a valid value.
-	dlist_iter	iter;

 
 
+ * autovac_balance_cost
+ *
+static void
+autovac_balance_cost(void)
+	dlist_iter	iter;
 
-	/*

 
 /*
@@ -2312,8 +2326,6 @@ do_autovacuum(void)
 		autovac_table *tab;
 		bool		isshared;
 		bool		skipit;
-		double		stdVacuumCostDelay;
-		int			stdVacuumCostLimit;
 		dlist_iter	iter;
 
 		CHECK_FOR_INTERRUPTS();
@@ -2416,31 +2428,17 @@ do_autovacuum(void)
 			continue;
 		}
 
-		/*

 
 		/* clean up memory before each iteration */
 		MemoryContextResetAndDeleteChildren(PortalContext);
@@ -2525,19 +2523,15 @@ deleted:
 
 		/*
 
 	/*
@@ -2569,6 +2563,8 @@ deleted:
 		{
 
 		LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
@@ -2804,8 +2800,6 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
 		int			freeze_table_age;
 		int			multixact_freeze_min_age;
 		int			multixact_freeze_table_age;
-		int			vac_cost_limit;
-		double		vac_cost_delay;
 		int			log_min_duration;
 
 		/*
@@ -2815,20 +2809,6 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
 		 * defaults, autovacuum's own first and plain vacuum second.
 		 */
 
@@ -2884,8 +2864,10 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
@@ -3377,10 +3359,14 @@ AutoVacuumShmemInit(void)
 
+
 		/* initialize the WorkerInfo free list */
 		for (i = 0; i < autovacuum_max_workers; i++)
 			dlist_push_head(&AutoVacuumShmem->av_freeWorkers,
 							&worker[i].wi_links);
+
+		pg_atomic_init_u32(&AutoVacuumShmem->av_nworkers_for_balance, 0);
+
 	}
 	else
 		Assert(found);
diff --git a/src/include/postmaster/autovacuum.h b/src/include/postmaster/autovacuum.h
index c140371b51..7b462866c9 100644
--- a/src/include/postmaster/autovacuum.h
+++ b/src/include/postmaster/autovacuum.h
@@ -65,6 +65,7 @@ extern void AutoVacWorkerFailed(void);

 #ifdef EXEC_BACKEND

From 28d33a9d5cab0d384a18dbc2bb4ad95da61741c3 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Mon, 27 Mar 2023 13:33:19 -0400
Subject: [PATCH v9 3/4] VACUUM reloads config file more often

Previously, VACUUM would not reload the configuration file. So, changes
to cost-based delay parameters could only take effect on the next
invocation of VACUUM.

Note that autovacuum is unaffected by this change. Autovacuum workers
overwrite the value of VacuumCostLimit and VacuumCostDelay with their
own WorkerInfo->wi_cost_limit and wi_cost_delay. Writing to their
wi_cost_delay more often makes reading wi_cost_delay without a lock to
update VacuumCostDelay an even worse idea.

Reviewed-by: Masahiko Sawada <[email-masked]>
Reviewed-by: Daniel Gustafsson <[email-masked]>
Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_buP5wzsho3qNw5o9_R0pF69FRM5hgCmr-mvXmGXwdA7A%40mail.gmail.com#5e6771d4cdca4db6efc2acec2dce0bc7
---
 src/backend/commands/vacuum.c | 62 ++++++++++++++++++++++++++++++-----
 1 file changed, 54 insertions(+), 8 deletions(-)

index eb126f2247..0e686c94b2 100644

 /*
 
 	const char *stmttype;
 
 	Assert(params != NULL);

 	/*

 
 				if (use_own_xacts)
 				{
 
@@ -2233,7 +2235,51 @@ vacuum_delay_point(void)
 	/* Always check for interrupts */
 	CHECK_FOR_INTERRUPTS();
 
-	if (VacuumCostInactive || InterruptPending)
+	if (InterruptPending ||
+		(VacuumCostInactive && !ConfigReloadPending))
+		return;
+
+	/*
+	 * Reload the configuration file if requested. This allows changes to
+	 * vacuum_cost_limit and vacuum_cost_delay to take effect while a table is
+	 * being vacuumed or analyzed. Analyze should not reload configuration
+	 * file if it is in an outer transaction, as GUC values shouldn't be
+	 * allowed to refer to some uncommitted state (e.g. database objects
+	 * created in this transaction).
+	 */
+	if (ConfigReloadPending && !analyze_in_outer_xact)
+	{
+		ConfigReloadPending = false;
+		ProcessConfigFile(PGC_SIGHUP);
+
+		/*
+		 * Autovacuum workers must restore the correct values of
+		 * VacuumCostLimit and VacuumCostDelay in case they were overwritten
+		 * by reload.
+		 */
+		AutoVacuumUpdateDelay();
+
+		/*
+		 * If configuration changes are allowed to impact VacuumCostInactive,
+		 * make sure it is updated.
+		 */
+		if (VacuumCostInactive == VACUUM_COST_INACTIVE_AND_LOCKED)
+			return;
+
+		if (VacuumCostDelay > 0)
+			VacuumCostInactive = VACUUM_COST_ACTIVE;
+		else
+		{
+			VacuumCostInactive = VACUUM_COST_INACTIVE_AND_UNLOCKED;
+			VacuumCostBalance = 0;
+		}
+	}
+
+	/*
+	 * If we disabled cost-based delays after reloading the config file,
+	 * return.
+	 */
+	if (VacuumCostInactive)
 		return;
 
 	/*
-- 
2.37.2

From 57a5284abc43fa5531574f998ac2ab8d038264d1 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Sat, 25 Mar 2023 12:05:18 -0400
Subject: [PATCH v9 2/4] Make VacuumCostActive failsafe-aware

 
 
 		return true;
 
 		in_vacuum = true;
 
+			/*

 	/*

-			if (VacuumCostActive)

 	VacuumPageMiss++;
-	if (VacuumCostActive)

 

 /* in tcop/postgres.c */
 
-- 
2.37.2