On Wed, Mar 15, 2023 at 1:14 AM Masahiko Sawada <sawada.m...@gmail.com> wrote:
> On Sat, Mar 11, 2023 at 8:11 AM Melanie Plageman
> <melanieplage...@gmail.com> wrote:
> > 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));
>     }

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

> > 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.

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.

> > > ---
> > >  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);

Ah, yes. You are right.

> 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.

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.

> > 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.

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?

> > > > 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?

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 <melanieplage...@gmail.com>
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 <sawada.m...@gmail.com>
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);
 
-		/* set the active cost parameters from the result of that */
+		AutoVacuumUpdateLimit();
 		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

Reply via email to