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

From: Kyotaro Horiguchi <[email-masked]>
Date: Wed, 29 Mar 2023 17:34:56 +0900 (JST)

At Wed, 29 Mar 2023 13:21:55 +0900 (JST), Kyotaro Horiguchi <[email-masked]> wrote in 
> So, sorry for the noise. I'll review it while this into cnosideration.

0003:

It's not this patche's fault, but I don't like the fact that the
variables used for GUC, VacuumCostDelay and VacuumCostLimit, are
updated outside the GUC mechanism. Also I don't like the incorrect
sorting of variables, where some working variables are referred to as
GUC parameters or vise versa.

Although it's somewhat unrelated to the goal of this patch, I think we
should clean up the code tidy before proceeding. Shouldn't we separate
the actual parameters from the GUC base variables, and sort out the
all related variaghble? (something like the attached, on top of your
patch.)

I have some comments on 0003 as-is.

+		tab->at_relopt_vac_cost_limit = avopts ?
+			avopts->vacuum_cost_limit : 0;
+		tab->at_relopt_vac_cost_delay = avopts ?
+			avopts->vacuum_cost_delay : -1;

The value is not used when do_balance is false, so I don't see a
specific reason for these variables to be different when avopts is
null.

+autovac_recalculate_workers_for_balance(void)
+{
+	dlist_iter	iter;
+	int			orig_nworkers_for_balance;
+	int			nworkers_for_balance = 0;
+
+	if (autovacuum_vac_cost_delay == 0 ||
+		(autovacuum_vac_cost_delay == -1 && VacuumCostDelay == 0))
 		return;
+	if (autovacuum_vac_cost_limit <= 0 && VacuumCostLimit <= 0)
+		return;
+

I'm not quite sure how these conditions relate to the need to count
workers that shares the global I/O cost. (Though I still believe this
funtion might not be necessary.)

+	if (av_relopt_cost_limit > 0)
+		VacuumCostLimit = av_relopt_cost_limit;
+	else
+	{
+		av_base_cost_limit = autovacuum_vac_cost_limit > 0 ?
+			autovacuum_vac_cost_limit : VacuumCostLimit;
+
+		AutoVacuumBalanceLimit();

I think each worker should use MyWorkerInfo->wi_dobalance to identyify
whether the worker needs to use balanced cost values.

+void
+AutoVacuumBalanceLimit(void)

I'm not sure this function needs to be a separate function.

(Sorry, timed out..)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index e9b683805a..f7ef7860ac 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -72,8 +72,22 @@ int			vacuum_multixact_freeze_min_age;
 int			vacuum_multixact_freeze_table_age;
 int			vacuum_failsafe_age;
 int			vacuum_multixact_failsafe_age;
+int			vacuum_cost_page_hit = 1;
+int			vacuum_cost_page_miss = 2;
+int			vacuum_cost_page_dirty = 20;
+int			vacuum_cost_limit = 200;
+double		vacuum_cost_delay = 0;

+/* working state for vacuum */
+int			VacuumCostBalance = 0;
+int			VacuumCostInactive = 1;
+int			VacuumCostLimit = 200;
+double		VacuumCostDelay = 0;
+int64		VacuumPageHit = 0;
+int64		VacuumPageMiss = 0;
+int64		VacuumPageDirty = 0;
+
 /* A few variables that don't seem worth passing around as parameters */
 static MemoryContext vac_context = NULL;
 static BufferAccessStrategy vac_strategy;
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index c8dae5465a..b475db9bfe 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -1832,7 +1832,7 @@ AutoVacuumUpdateLimit(void)
 	else
 	{
 		av_base_cost_limit = autovacuum_vac_cost_limit > 0 ?
-			autovacuum_vac_cost_limit : VacuumCostLimit;
+			autovacuum_vac_cost_limit : vacuum_cost_limit;
 
 		AutoVacuumBalanceLimit();
 	}
@@ -1866,7 +1866,7 @@ AutoVacuumBalanceLimit(void)
 	Assert(nworkers_for_balance > 0);
 
 	total_cost_limit = autovacuum_vac_cost_limit > 0 ?
-		autovacuum_vac_cost_limit : av_base_cost_limit;
+		autovacuum_vac_cost_limit : vacuum_cost_limit;
 
 	balanced_cost_limit = total_cost_limit / nworkers_for_balance;
 
@@ -1888,10 +1888,10 @@ autovac_recalculate_workers_for_balance(void)
 	int			nworkers_for_balance = 0;
 
 	if (autovacuum_vac_cost_delay == 0 ||
-		(autovacuum_vac_cost_delay == -1 && VacuumCostDelay == 0))
+		(autovacuum_vac_cost_delay == -1 && vacuum_cost_limit == 0))
 		return;
 
-	if (autovacuum_vac_cost_limit <= 0 && VacuumCostLimit <= 0)
+	if (autovacuum_vac_cost_limit <= 0 && vacuum_cost_limit <= 0)
 		return;
 
 	orig_nworkers_for_balance =
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 6d3dd26fc7..4524df23c4 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -39,6 +39,7 @@
 #include "catalog/catalog.h"
 #include "catalog/storage.h"
 #include "catalog/storage_xlog.h"
+#include "commands/vacuum.h"
 #include "executor/instrument.h"
 #include "lib/binaryheap.h"
 #include "miscadmin.h"
@@ -894,7 +895,7 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
 			VacuumPageHit++;
 
 			if (!VacuumCostInactive)
-				VacuumCostBalance += VacuumCostPageHit;
+				VacuumCostBalance += vacuum_cost_page_hit;
 
 			TRACE_POSTGRESQL_BUFFER_READ_DONE(forkNum, blockNum,
 											  smgr->smgr_rlocator.locator.spcOid,
@@ -1099,7 +1100,7 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
 
 	VacuumPageMiss++;
 	if (!VacuumCostInactive)
-		VacuumCostBalance += VacuumCostPageMiss;
+		VacuumCostBalance += vacuum_cost_page_miss;
 
 	TRACE_POSTGRESQL_BUFFER_READ_DONE(forkNum, blockNum,
 									  smgr->smgr_rlocator.locator.spcOid,
@@ -1673,7 +1674,7 @@ MarkBufferDirty(Buffer buffer)
 		VacuumPageDirty++;
 		pgBufferUsage.shared_blks_dirtied++;
 		if (!VacuumCostInactive)
-			VacuumCostBalance += VacuumCostPageDirty;
+			VacuumCostBalance += vacuum_cost_page_dirty;
 	}
 }
 
@@ -4200,7 +4201,7 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
 			VacuumPageDirty++;
 			pgBufferUsage.shared_blks_dirtied++;
 			if (!VacuumCostInactive)
-				VacuumCostBalance += VacuumCostPageDirty;
+				VacuumCostBalance += vacuum_cost_page_dirty;
 		}
 	}
 }
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 608ebb9182..dbf463c6e1 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -138,16 +138,3 @@ int			MaxConnections = 100;
 int			max_worker_processes = 8;
 int			max_parallel_workers = 8;
 int			MaxBackends = 0;
-
-int			VacuumCostPageHit = 1;	/* GUC parameters for vacuum */
-int			VacuumCostPageMiss = 2;
-int			VacuumCostPageDirty = 20;
-int			VacuumCostLimit = 200;
-double		VacuumCostDelay = 0;
-
-int64		VacuumPageHit = 0;
-int64		VacuumPageMiss = 0;
-int64		VacuumPageDirty = 0;
-
-int			VacuumCostBalance = 0;	/* working state for vacuum */
-int			VacuumCostInactive = 1;
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 8062589efd..179f39ab9c 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2379,7 +2379,7 @@ struct config_int ConfigureNamesInt[] =
 			gettext_noop("Vacuum cost for a page found in the buffer cache."),
 			NULL
 		},
-		&VacuumCostPageHit,
+		&vacuum_cost_page_hit,
 		1, 0, 10000,
 		NULL, NULL, NULL
 	},
@@ -2389,7 +2389,7 @@ struct config_int ConfigureNamesInt[] =
 			gettext_noop("Vacuum cost for a page not found in the buffer cache."),
 			NULL
 		},
-		&VacuumCostPageMiss,
+		&vacuum_cost_page_miss,
 		2, 0, 10000,
 		NULL, NULL, NULL
 	},
@@ -2399,7 +2399,7 @@ struct config_int ConfigureNamesInt[] =
 			gettext_noop("Vacuum cost for a page dirtied by vacuum."),
 			NULL
 		},
-		&VacuumCostPageDirty,
+		&vacuum_cost_page_dirty,
 		20, 0, 10000,
 		NULL, NULL, NULL
 	},
@@ -2409,7 +2409,7 @@ struct config_int ConfigureNamesInt[] =
 			gettext_noop("Vacuum cost amount available before napping."),
 			NULL
 		},
-		&VacuumCostLimit,
+		&vacuum_cost_limit,
 		200, 1, 10000,
 		NULL, NULL, NULL
 	},
@@ -3701,7 +3701,7 @@ struct config_real ConfigureNamesReal[] =
 			NULL,
 			GUC_UNIT_MS
 		},
-		&VacuumCostDelay,
+		&vacuum_cost_delay,
 		0, 0, 100,
 		NULL, NULL, NULL
 	},
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 5c3e250b06..54842a75f9 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -300,6 +300,11 @@ extern PGDLLIMPORT int vacuum_multixact_freeze_min_age;
 extern PGDLLIMPORT int vacuum_multixact_freeze_table_age;
 extern PGDLLIMPORT int vacuum_failsafe_age;
 extern PGDLLIMPORT int vacuum_multixact_failsafe_age;
+extern PGDLLIMPORT int vacuum_cost_limit;
+extern PGDLLIMPORT double vacuum_cost_delay;
+extern PGDLLIMPORT int vacuum_cost_page_hit;
+extern PGDLLIMPORT int vacuum_cost_page_miss;
+extern PGDLLIMPORT int vacuum_cost_page_dirty;
 
 /* Variables for cost-based parallel vacuum */
 
@@ -312,7 +317,15 @@ typedef enum VacuumCostStatus
 
 extern PGDLLIMPORT pg_atomic_uint32 *VacuumSharedCostBalance;
 extern PGDLLIMPORT pg_atomic_uint32 *VacuumActiveNWorkers;
+
 extern PGDLLIMPORT int VacuumCostBalanceLocal;
+extern PGDLLIMPORT int VacuumCostBalance;
+extern PGDLLIMPORT int VacuumCostInactive;
+extern PGDLLIMPORT int VacuumCostLimit;
+extern PGDLLIMPORT double VacuumCostDelay;
+extern PGDLLIMPORT int64 VacuumPageHit;
+extern PGDLLIMPORT int64 VacuumPageMiss;
+extern PGDLLIMPORT int64 VacuumPageDirty;

 /* in commands/vacuum.c */
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 33e22733ae..cf0cc919cf 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -263,18 +263,10 @@ extern PGDLLIMPORT double hash_mem_multiplier;
 extern PGDLLIMPORT int maintenance_work_mem;
 extern PGDLLIMPORT int max_parallel_maintenance_workers;
 
-extern PGDLLIMPORT int VacuumCostPageHit;
-extern PGDLLIMPORT int VacuumCostPageMiss;
-extern PGDLLIMPORT int VacuumCostPageDirty;
-extern PGDLLIMPORT int VacuumCostLimit;
-extern PGDLLIMPORT double VacuumCostDelay;
 
-extern PGDLLIMPORT int64 VacuumPageHit;
-extern PGDLLIMPORT int64 VacuumPageMiss;
-extern PGDLLIMPORT int64 VacuumPageDirty;
-
-extern PGDLLIMPORT int VacuumCostBalance;
-extern PGDLLIMPORT int VacuumCostInactive;
+/*****************************************************************************
+ *	  globals.h --															 *
+ *****************************************************************************/
 
 /* in tcop/postgres.c */

---

From: Melanie Plageman <[email-masked]>
Date: Wed, 29 Mar 2023 16:01:00 -0400

Thanks for the detailed review!

On Tue, Mar 28, 2023 at 11:09 PM Kyotaro Horiguchi
<[email-masked]> wrote:
>
> At Tue, 28 Mar 2023 20:35:28 -0400, Melanie Plageman <[email-masked]> wrote in
> > On Tue, Mar 28, 2023 at 4:21 AM Kyotaro Horiguchi <[email-masked]> wrote:
> > >
> > > At Mon, 27 Mar 2023 14:12:03 -0400, Melanie Plageman <[email-masked]> wrote in
> > >
> > > 0002:
> > >
> > > I felt a bit uneasy on this. It seems somewhat complex (and makes the
> > > succeeding patches complex),
> >
> > Even if we introduced a second global variable to indicate that failsafe
> > mode has been engaged, we would still require the additional checks
> > of VacuumCostInactive.
> >
> > > has confusing names,
> >
> > I would be happy to rename the values of the enum to make them less
> > confusing. Are you thinking "force" instead of "locked"?
> > maybe:
> > VACUUM_COST_FORCE_INACTIVE and
> > VACUUM_COST_INACTIVE
> > ?
> >
> > > and doesn't seem like self-contained.
> >
> > By changing the variable from VacuumCostActive to VacuumCostInactive, I
> > have kept all non-vacuum code from having to distinguish between it
> > being inactive due to failsafe mode or due to user settings.
>
> My concern is that VacuumCostActive is logic-inverted and turned into
> a ternary variable in a subtle way. The expression
> "!VacuumCostInactive" is quite confusing. (I sometimes feel the same
> way about "!XLogRecPtrIsInvalid(lsn)", and I believe most people write
> it with another macro like "lsn != InvalidXLogrecPtr"). Additionally,
> the constraint in this patch will be implemented as open code.  So I
> wanted to suggest something like the attached. The main idea is to use
> a wrapper function to enforce the restriction, and by doing so, we
> eliminated the need to make the variable into a ternary without a good
> reason.

So, the rationale for making it a ternary is that the variable is the
combination of two pieces of information which has only has 3 valid
states:
failsafe inactive + cost active = cost active
failsafe inactive + cost inactive = cost inactive
failsafe active + cost inactive = cost inactive and locked
the fourth is invalid
failsafe active + cost active = invalid
That is harder to enforce with two variables.
Also, the two pieces of information are not meaningful individually.
So, I thought it made sense to make a single variable.

Your suggested patch introduces an additional variable which shadows
LVRelState->failsafe_active but doesn't actually get set/reset at all of
the correct places. If we did introduce a second global variable, I
don't think we should also keep LVRelState->failsafe_active, as keeping
them in sync will be difficult.

As for the double negative (!VacuumCostInactive), I agree that it is not
ideal, however, if we use a ternary and keep VacuumCostActive, there is
no way for non-vacuum code to treat it as a boolean.
With the ternary VacuumCostInactive, only vacuum code has to know about
the distinction between inactive+failsafe active and inactive+failsafe
inactive.

As for the setter function, I think that having a function to set
VacuumCostActive based on failsafe_active is actually doing more harm
than good. Only vacuum code has to know about the distinction as it is,
so we aren't really saving any trouble (there would really only be two
callers of the suggested function). And, since the function hides
whether or not VacuumCostActive was actually set to the passed-in value,
we can't easily do other necessary maintenance -- like zero out
VacuumCostBalance if we disabled vacuum cost.

> > > 0003:
> > >
> > > +        * 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).
> > >
> > > I'm not sure GUC reload is or should be related to transactions. For
> > > instance, work_mem can be changed by a reload during a transaction
> > > unless it has been set in the current transaction. I don't think we
> > > need to deliberately suppress changes in variables caused by realods
> > > during transactions only for analzye. If analyze doesn't like changes
> > > to certain GUC variables, their values should be snapshotted before
> > > starting the process.
> >
> > Currently, we only reload the config file in top-level statements. We
> > don't reload the configuration file from within a nested transaction
> > command. BEGIN starts a transaction but not a transaction command. So
> > BEGIN; ANALYZE; probably wouldn't violate this rule. But it is simpler
> > to just forbid reloading when it is not a top-level transaction command.
> > I have updated the comment to reflect this.
>
> I feel it's a bit fragile. We may not be able to manage the reload
> timeing perfectly. I think we might accidentally add a reload
> timing. In that case, the assumption could break. In most cases, I
> think we use snapshotting in various ways to avoid unintended variable
> changes. (And I beilieve the analyze code also does that.)

I'm not sure I fully understand the problem you are thinking of. What do
you mean about managing the reload timing? Are you suggesting there is a
problem with excluding analzye in an outer transaction from doing the
reload or with doing the reload during vacuum and analyze when they are
top-level statements?

And, by snapshotting do you mean how vacuum_rel() and do_analyze_rel() do
NewGUCNestLevel() so that they can then do AtEOXact_GUC() and rollback
guc changes done during that operation?
How are you envisioning that being used here?

On Wed, Mar 29, 2023 at 2:00 AM Kyotaro Horiguchi
<[email-masked]> wrote:
> > autovacuum.c:2893
> >               /*
> >                * If any of the cost delay parameters has been set individually for
> >                * this table, disable the balancing algorithm.
> >                */
> >               tab->at_dobalance =
> >                       !(avopts && (avopts->vacuum_cost_limit > 0 ||
> >                                                avopts->vacuum_cost_delay > 0));
> >
> Then I found that the code is quite confusing as it is.
>
> For the tables that don't have cost_delay and cost_limit specified
> indificually, at_vacuum_cost_limit and _delay store the system global
> values detemined by GUCs. wi_cost_delay, _limit and _limit_base stores
> the same values with them. As the result I concluded tha
> autovac_balance_cost() does exactly what Melanie's patch does, except
> that nworkers_for_balance is not stored in shared memory.
>
> I discovered that commit 1021bd6a89 brought in do_balance.
>
> >    Since the mechanism is already complicated, just disable it for those
> >    cases rather than trying to make it cope.  There are undesirable
>
> After reading this, I get why the code is so complex.  It is a remnant
> of when balancing was done with tables that had individually specified
> cost parameters. And I found the following description in the doc.
>
> https://www.postgresql.org/docs/devel/routine-vacuuming.html
> > When multiple workers are running, the autovacuum cost delay
> > parameters (see Section 20.4.4) are “balanced” among all the running
> > workers, so that the total I/O impact on the system is the same
> > regardless of the number of workers actually running. However, any
> > workers processing tables whose per-table
> > autovacuum_vacuum_cost_delay or autovacuum_vacuum_cost_limit storage
> > parameters have been set are not considered in the balancing
> > algorithm.
>
> The initial balancing mechanism was brought in by e2a186b03c back in
> 2007. The balancing code has had that unnecessarily complexity ever
> since.
>
> Since I can't think of a better idea than Melanie's proposal for
> handling this code, I'll keep reviewing it with that approach in mind.

Thanks for doing this archaeology. I didn't know the history of dobalance
and hadn't looked into 1021bd6a89.
I was a bit confused by why dobalance was false even if only table
option cost delay is set and not table option cost limit.

I think we can retain this behavior for now, but it may be worth
re-examining in the future.

> all related variaghble? (something like the attached, on top of your

So, I agree we should separate the parameters used in the code from the
GUC variables -- since there are multiple users with different needs
(autovac workers, parallel vac workers, and vacuum). However, I was
hesitant to tackle that here.

I'm not sure how these changes will impact extensions that rely on
these vacuum parameters and their direct relationship to the guc values.

In your patch, you didn't update the parameter with the guc value of
vacuum_cost_limit and vacuum_cost_delay, but were we to do so, we would
need to make sure it was updated every time after a config reload. This
isn't hard to do in the current code, but I'm not sure how we can ensure
that future callers of ProcessConfigFile() in vacuum code always update
these values afterward. Perhaps we could add some after_reload hook?
Which does seem like a larger project.

> specific reason for these variables to be different when avopts is

Actually we need to set these to 0 and -1, because we set
av_relopt_cost_limit and av_relopt_cost_delay with them and those values
are checked regardless of wi_dobalance.

We need to do this because we want to use the correct value to override
VacuumCostLimit and VacuumCostDelay. wi_dobalance may be false because
we have a table option cost delay but we have no table option cost
limit. When we override VacuumCostDelay, we want to use the table option
value but when we override VacuumCostLimit, we want to use the regular
value. We need these initialized to values that will allow us to do
that.

> workers that shares the global I/O cost.

Ah, this is a good point, we should still keep this number up-to-date
even if the costs are disabled at the time we are checking it in case
cost-based delays are re-enabled later before we recalculate this
number.  I had this code originally because autovac_balance_cost() would
exit early if cost-based delays were disabled -- but this only worked
because they couldn't be re-enabled during vacuuming a table and
autovac_balance_cost() was called always in between vacuuming tables.

I've removed these lines.

And perhaps there is an argument for calling
autovac_recalculate_workers_for_balance() in vacuum_delay_point() after
reloading the config file...
I have not done so in attached version.

> (Though I still believe this funtion might not be necessary.)

I don't see how we can do without this function. We need an up-to-date
count of the number of autovacuum workers vacuuming tables which do not
have vacuum cost-related table options.

> whether the worker needs to use balanced cost values.

Ah, there is a bug here. I have fixed it by making wi_dobalance an
atomic flag so that we can check it before calling
AutoVacuumBalanceLimit() (without taking a lock).

I don't see other (non-test code) callers using atomic flags, so I can't
tell if we need to loop to ensure that pg_atomic_test_set_flag() returns
true.

> I'm not sure this function needs to be a separate function.

We need to call it more often than we can call AutoVacuumUpdateLimit(),
so the logic needs to be separate. Are you suggesting we inline the
logic in the two places it is needed?

v11 attached with updates mentioned above.

- Melanie
From a1db301c122641acd297a05d29bcb32bc7b769e2 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Sat, 25 Mar 2023 14:14:55 -0400
Subject: [PATCH v11 3/3] 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.

Reviewed-by: Kyotaro Horiguchi <[email-masked]>
---
 src/backend/commands/vacuum.c       |  22 ++-
 src/backend/postmaster/autovacuum.c | 286 +++++++++++++++-------------
 src/include/postmaster/autovacuum.h |   2 +
 3 files changed, 175 insertions(+), 135 deletions(-)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 7e3a8e404e..e9b683805a 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -2241,10 +2241,10 @@ 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 we currently only allow
-	 * configuration reload when in top-level statements.
+	 * [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 we
+	 * currently only allow configuration reload when in top-level statements.
 	 */
 	if (ConfigReloadPending && !analyze_in_outer_xact)
 	{
@@ -2257,10 +2257,12 @@ vacuum_delay_point(void)
 		 * by reload.
 		 */
 		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;
@@ -2311,8 +2313,14 @@ vacuum_delay_point(void)
 
 		VacuumCostBalance = 0;
 
-		/* update balance values for workers */
-		AutoVacuumUpdateDelay();
+		/*
+		 * Balance and 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.
+		 */
+		AutoVacuumBalanceLimit();
 
 		/* 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 585d28148c..9775764fc4 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -139,6 +139,10 @@ int			Log_autovacuum_min_duration = 600000;
 static bool am_autovacuum_launcher = false;
 static bool am_autovacuum_worker = false;
 
+static double av_relopt_cost_delay = -1;
+static int	av_relopt_cost_limit = 0;
+static int	av_base_cost_limit = 0;
+
 /* Flags set by signal handlers */
 static volatile sig_atomic_t got_SIGUSR2 = false;
 
@@ -189,8 +193,8 @@ typedef struct autovac_table
 {
 	Oid			at_relid;
 	VacuumParams at_params;
-	double		at_vacuum_cost_delay;
-	int			at_vacuum_cost_limit;
+	double		at_relopt_vac_cost_delay;
+	int			at_relopt_vac_cost_limit;
 	bool		at_dobalance;
 	bool		at_sharedrel;
 	char	   *at_relname;
@@ -209,7 +213,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
@@ -223,11 +227,8 @@ typedef struct WorkerInfoData
 	Oid			wi_tableoid;
 	PGPROC	   *wi_proc;
 	TimestampTz wi_launchtime;
-	bool		wi_dobalance;
+	pg_atomic_flag wi_dobalance;
 	bool		wi_sharedrel;
-	double		wi_cost_delay;
-	int			wi_cost_limit;
-	int			wi_cost_limit_base;
 } WorkerInfoData;
 
 typedef struct WorkerInfoData *WorkerInfo;
@@ -273,6 +274,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_nworkersForBalance 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 +289,7 @@ typedef struct
 	dlist_head	av_runningWorkers;
 	WorkerInfo	av_startingWorker;
 	AutoVacuumWorkItem av_workItems[NUM_WORKITEMS];
+	pg_atomic_uint32 av_nworkersForBalance;
 } AutoVacuumShmemStruct;
 
 static AutoVacuumShmemStruct *AutoVacuumShmem;
@@ -319,7 +323,7 @@ static void launch_worker(TimestampTz now);
 static List *get_database_list(void);
 static void rebuild_database_list(Oid newdb);
 static int	db_comparator(const void *a, const void *b);
-static void autovac_balance_cost(void);
+static void autovac_recalculate_workers_for_balance(void);
 
 static void do_autovacuum(void);
 static void FreeWorkerInfo(int code, Datum arg);
@@ -670,7 +674,7 @@ AutoVacLauncherMain(int argc, char *argv[])
 			{
 				LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
 				AutoVacuumShmem->av_signal[AutoVacRebalance] = false;
-				autovac_balance_cost();
+				autovac_recalculate_workers_for_balance();
 				LWLockRelease(AutovacuumLock);
 			}
 
@@ -820,8 +824,8 @@ HandleAutoVacLauncherInterrupts(void)
 			AutoVacLauncherShutdown();
 
 		/* rebalance in case the default cost parameters changed */
-		LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
-		autovac_balance_cost();
+		LWLockAcquire(AutovacuumLock, LW_SHARED);
+		autovac_recalculate_workers_for_balance();
 		LWLockRelease(AutovacuumLock);
 
 		/* rebuild the list in case the naptime changed */
@@ -1755,10 +1759,7 @@ FreeWorkerInfo(int code, Datum arg)
 		MyWorkerInfo->wi_sharedrel = false;
 		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;
+		pg_atomic_clear_flag(&MyWorkerInfo->wi_dobalance);
 		dlist_push_head(&AutoVacuumShmem->av_freeWorkers,
 						&MyWorkerInfo->wi_links);
 		/* not mine anymore */
@@ -1773,100 +1774,140 @@ 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_relopt_cost_delay >= 0)
+		VacuumCostDelay = av_relopt_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;
 	}
 }
 
+
 /*
- * autovac_balance_cost
- *		Recalculate the cost limit setting for each active worker.
- *
- * Caller must hold the AutovacuumLock in exclusive mode.
+ * This must be called directly after a config reload before using the value of
+ * VacuumCostLimit and before calling AutoVacuumBalanceLimit(), as it uses the
+ * value of VacuumCostLimit to determine what the base av_base_cost_limit
+ * should be. AutoVacuumBalanceLimit() will override the value of
+ * VacuumCostLimit, so calling it multiple times after a config reload is
+ * incorrect.
  */
-static void
-autovac_balance_cost(void)
+void
+AutoVacuumUpdateLimit(void)
 {
+	if (!am_autovacuum_worker)
+		return;
+
 	/*
-	 * 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;
-
-	/* not set? nothing to do */
-	if (vac_cost_limit <= 0 || vac_cost_delay <= 0)
-		return;
-
-	/* calculate the total base cost limit of participating active workers */
-	cost_total = 0.0;
-	dlist_foreach(iter, &AutoVacuumShmem->av_runningWorkers)
+	if (av_relopt_cost_limit > 0)
+		VacuumCostLimit = av_relopt_cost_limit;
+	else
 	{
-		WorkerInfo	worker = dlist_container(WorkerInfoData, wi_links, iter.cur);
+		av_base_cost_limit = autovacuum_vac_cost_limit > 0 ?
+			autovacuum_vac_cost_limit : VacuumCostLimit;
 
-		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;
+		AutoVacuumBalanceLimit();
 	}
+}
 
-	/* there are no cost limits -- nothing to do */
-	if (cost_total <= 0)
+/*
+ * 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. After a config reload, they
+ * must call AutoVacuumUpdateLimit() which will call AutoVacuumBalanceLimit(),
+ * in case VacuumCostLimit was overwritten.
+ */
+void
+AutoVacuumBalanceLimit(void)
+{
+	int			nworkers_for_balance;
+	int			total_cost_limit;
+	int			balanced_cost_limit;
+
+	if (!MyWorkerInfo)
 		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;
+	if (pg_atomic_unlocked_test_flag(&MyWorkerInfo->wi_dobalance))
+		return;
+
+	Assert(av_base_cost_limit > 0);
+
+	nworkers_for_balance = pg_atomic_read_u32(
+							&AutoVacuumShmem->av_nworkersForBalance);
+
+	/* There is at least 1 autovac worker (this worker). */
+	Assert(nworkers_for_balance > 0);
+
+	total_cost_limit = autovacuum_vac_cost_limit > 0 ?
+		autovacuum_vac_cost_limit : av_base_cost_limit;
+
+	balanced_cost_limit = total_cost_limit / nworkers_for_balance;
+
+	VacuumCostLimit = Max(Min(balanced_cost_limit, total_cost_limit), 1);
+}
+
+/*
+ * autovac_recalculate_workers_for_balance
+ *		Recalculate the number of workers to consider, given table options and
+ *		the current number of active workers.
+ *
+ * Caller must hold the AutovacuumLock in at least shared mode to access
+ * worker->wi_proc.
+ */
+static void
+autovac_recalculate_workers_for_balance(void)
+{
+	dlist_iter	iter;
+	int			orig_nworkers_for_balance;
+	int			nworkers_for_balance = 0;
+
+	orig_nworkers_for_balance =
+		pg_atomic_read_u32(&AutoVacuumShmem->av_nworkersForBalance);
+
 	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 ||
+			pg_atomic_unlocked_test_flag(&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++;
 	}
+
+	if (nworkers_for_balance != orig_nworkers_for_balance)
+		pg_atomic_write_u32(&AutoVacuumShmem->av_nworkersForBalance,
+							nworkers_for_balance);
 }
 
 /*
@@ -2312,8 +2353,6 @@ do_autovacuum(void)
 		autovac_table *tab;
 		bool		isshared;
 		bool		skipit;
-		double		stdVacuumCostDelay;
-		int			stdVacuumCostLimit;
 		dlist_iter	iter;
 
 		CHECK_FOR_INTERRUPTS();
@@ -2417,30 +2456,29 @@ do_autovacuum(void)
 		}
 
 		/*
-		 * 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().
+		 * Save the cost-related table options in global variables for
+		 * reference when updating VacuumCostLimit and VacuumCostDelay during
+		 * vacuuming this table.
 		 */
-		stdVacuumCostDelay = VacuumCostDelay;
-		stdVacuumCostLimit = VacuumCostLimit;
+		av_relopt_cost_limit = tab->at_relopt_vac_cost_limit;
+		av_relopt_cost_delay = tab->at_relopt_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;
+		if (tab->at_dobalance)
+			pg_atomic_test_set_flag(&MyWorkerInfo->wi_dobalance);
+		else
+			pg_atomic_clear_flag(&MyWorkerInfo->wi_dobalance);
 
-		/* do a balance */
-		autovac_balance_cost();
+		LWLockAcquire(AutovacuumLock, LW_SHARED);
+		autovac_recalculate_workers_for_balance();
+		LWLockRelease(AutovacuumLock);
 
-		/* set the active cost parameters from the result of that */
+		/*
+		 * We wait until this point to update cost delay and cost limit
+		 * values, even though we reloaded the configuration file above, so
+		 * that we can take into account the cost-related table options.
+		 */
 		AutoVacuumUpdateDelay();
-
-		/* done */
-		LWLockRelease(AutovacuumLock);
+		AutoVacuumUpdateLimit();
 
 		/* clean up memory before each iteration */
 		MemoryContextResetAndDeleteChildren(PortalContext);
@@ -2525,19 +2563,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, unset wi_dobalance 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 +2603,8 @@ deleted:
 		{
 			ConfigReloadPending = false;
 			ProcessConfigFile(PGC_SIGHUP);
+			AutoVacuumUpdateDelay();
+			AutoVacuumUpdateLimit();
 		}
 
 		LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
@@ -2804,8 +2840,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 +2849,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
@@ -2884,8 +2904,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_relopt_vac_cost_limit = avopts ?
+			avopts->vacuum_cost_limit : 0;
+		tab->at_relopt_vac_cost_delay = avopts ?
+			avopts->vacuum_cost_delay : -1;
 		tab->at_relname = NULL;
 		tab->at_nspname = NULL;
 		tab->at_datname = NULL;
@@ -3377,10 +3399,18 @@ 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);
+
+			pg_atomic_init_flag(&worker[i].wi_dobalance);
+		}
+
+		pg_atomic_init_u32(&AutoVacuumShmem->av_nworkersForBalance, 0);
+
 	}
 	else
 		Assert(found);
diff --git a/src/include/postmaster/autovacuum.h b/src/include/postmaster/autovacuum.h
index c140371b51..80bdfb2cc0 100644
--- a/src/include/postmaster/autovacuum.h
+++ b/src/include/postmaster/autovacuum.h
@@ -64,7 +64,9 @@ extern int	StartAutoVacWorker(void);
 extern void AutoVacWorkerFailed(void);
 
 /* autovacuum cost-delay balancer */
+extern void AutoVacuumBalanceLimit(void);
 extern void AutoVacuumUpdateDelay(void);
+extern void AutoVacuumUpdateLimit(void);
 
 #ifdef EXEC_BACKEND
 extern void AutoVacLauncherMain(int argc, char *argv[]) pg_attribute_noreturn();
-- 
2.37.2

From a46f13517d165479ec518b46fe98591aa75f35d6 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Sat, 25 Mar 2023 12:05:18 -0400
Subject: [PATCH v11 1/3] 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         | 24 +++++++++++++++++++++---
 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, 42 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 c54360a6a0..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,8 @@ vacuum(List *relations, VacuumParams *params,
 	PG_FINALLY();
 	{
 		in_vacuum = false;
-		VacuumCostActive = false;
+		VacuumCostInactive = VACUUM_COST_INACTIVE_AND_UNLOCKED;
+		VacuumCostBalance = 0;
 	}
 	PG_END_TRY();
 
@@ -2215,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 fe029d2ea6..495ee8f815 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;
 	}
 }
@@ -4189,7 +4189,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 3f4a39ff366f491ecdf2361ae11704bc98175a30 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Mon, 27 Mar 2023 13:33:19 -0400
Subject: [PATCH v11 2/3] 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.

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

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 | 61 ++++++++++++++++++++++++++++++-----
 1 file changed, 53 insertions(+), 8 deletions(-)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index eb126f2247..7e3a8e404e 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())
@@ -544,7 +545,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)
 				{
@@ -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,50 @@ 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 we currently only allow
+	 * configuration reload when in top-level statements.
+	 */
+	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: Masahiko Sawada <[email-masked]>
Date: Thu, 30 Mar 2023 11:57:05 +0900

Hi,

Thank you for updating the patches.

> the distinction between inactive+failsafe active and inactive+failsafe

As another idea, why don't we use macros for that? For example,
suppose VacuumCostStatus is like:

typedef enum VacuumCostStatus
{
    VACUUM_COST_INACTIVE_LOCKED = 0,
    VACUUM_COST_INACTIVE,
    VACUUM_COST_ACTIVE,
} VacuumCostStatus;
VacuumCostStatus VacuumCost;

non-vacuum code can use the following macros:

#define VacuumCostActive() (VacuumCost == VACUUM_COST_ACTIVE)
#define VacuumCostInactive() (VacuumCost <= VACUUM_COST_INACTIVE) //
or we can use !VacuumCostActive() instead.

Or is there any reason why we need to keep VacuumCostActive and treat
it as a boolean?

Regards,

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

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 30 Mar 2023 21:26:47 +0200

> On 30 Mar 2023, at 04:57, Masahiko Sawada <[email-masked]> wrote:

> or we can use !VacuumCostActive() instead.

I'm in favor of something along these lines.  A variable with a name that
implies a boolean value (active/inactive) but actually contains a tri-value is
easily misunderstood.  A VacuumCostState tri-value variable (or a better name)
with a set of convenient macros for extracting the boolean active/inactive that
most of the code needs to be concerned with would more for more readable code I
think.

--
Daniel Gustafsson

---

From: Melanie Plageman <[email-masked]>
Date: Fri, 31 Mar 2023 10:31:10 -0400

> most of the code needs to be concerned with would more for more readable code I

The macros are very error-prone. I was just implementing this idea and
mistakenly tried to set the macro instead of the variable in multiple
places. Avoiding this involves another set of macros, and, in the end, I
think the complexity is much worse. Given the reviewers' uniform dislike
of VacuumCostInactive, I favor going back to two variables
(VacuumCostActive + VacuumFailsafeActive) and moving
LVRelState->failsafe_active to the global VacuumFailsafeActive.

I will reimplement this in the next version.

On the subject of globals, the next version will implement
Horiguchi-san's proposal to separate GUC variables from the globals used
in the code (quoted below). It should hopefully reduce the complexity of
this patchset.

- Melanie

---

From: Melanie Plageman <[email-masked]>
Date: Fri, 31 Mar 2023 15:09:21 -0400

> > all related variaghble? (something like the attached, on top of your

Attached is v12. It has a number of updates, including a commit to
separate VacuumCostLimit and VacuumCostDelay from the gucs
vacuum_cost_limit and vacuum_cost_delay, and a return to
VacuumCostActive.

- Melanie
From 106f5db65846e0497945b6171bdc29f5727aadc3 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Fri, 31 Mar 2023 10:38:39 -0400
Subject: [PATCH v12 1/4] Make vacuum's failsafe_active a global

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, elevate
LVRelState->failsafe_active to a global, VacuumFailsafeActive, which
will be checked when determining whether or not to re-enable vacuum
cost-related delays.
---
 src/backend/access/heap/vacuumlazy.c  | 16 +++++++---------
 src/backend/commands/vacuum.c         |  1 +
 src/backend/commands/vacuumparallel.c |  1 +
 src/include/commands/vacuum.h         |  1 +
 4 files changed, 10 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..0e1dbeec70 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -85,6 +85,7 @@ static BufferAccessStrategy vac_strategy;
 pg_atomic_uint32 *VacuumSharedCostBalance = NULL;
 pg_atomic_uint32 *VacuumActiveNWorkers = NULL;
 int			VacuumCostBalanceLocal = 0;
+bool		VacuumFailsafeActive = false;
 
 /* non-export function prototypes */
 static List *expand_vacuum_rel(VacuumRelation *vrel, int options);
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/include/commands/vacuum.h b/src/include/commands/vacuum.h
index bdfd96cfec..7b8ee21788 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -306,6 +306,7 @@ extern PGDLLIMPORT pg_atomic_uint32 *VacuumSharedCostBalance;
 extern PGDLLIMPORT pg_atomic_uint32 *VacuumActiveNWorkers;
 extern PGDLLIMPORT int VacuumCostBalanceLocal;
 
+extern bool VacuumFailsafeActive;
 
 /* in commands/vacuum.c */
 extern void ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel);
-- 
2.37.2

From 795776af97d5f3ab05e48c7b78367bf6290e7ad4 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Mon, 27 Mar 2023 13:33:19 -0400
Subject: [PATCH v12 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.

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

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 instead of using
potentially refreshed values of autovacuum_vacuum_cost_limit and
autovacuum_vacuum_cost_delay. Locking considerations and needed updates
to the worker balancing logic make enabling this feature for autovacuum
worthy of an independent commit.

Reviewed-by: Masahiko Sawada <[email-masked]>
Reviewed-by: Kyotaro Horiguchi <[email-masked]>
Reviewed-by: Daniel Gustafsson <[email-masked]>
Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7%2BDXhrjDA0gw%40mail.gmail.com
---
 src/backend/commands/vacuum.c         | 46 +++++++++++++++++++++------
 src/backend/commands/vacuumparallel.c |  3 +-
 src/backend/postmaster/autovacuum.c   | 18 +++++++++++
 3 files changed, 55 insertions(+), 12 deletions(-)

index 2c3afd4ff6..a288c402a9 100644
@@ -78,6 +79,7 @@ int			vacuum_cost_limit;
 
 /*
  * Variables for cost-based vacuum delay. The defaults differ between
@@ -325,8 +327,7 @@ vacuum(List *relations, VacuumParams *params,
 	static bool in_vacuum = false;
 
 	const char *stmttype;
 
 	Assert(params != NULL);
 
@@ -343,10 +344,10 @@ vacuum(List *relations, VacuumParams *params,
 
 	/*
@@ -468,7 +469,7 @@ vacuum(List *relations, VacuumParams *params,
@@ -486,7 +487,7 @@ vacuum(List *relations, VacuumParams *params,
 
 		/* ActiveSnapshot is not set by autovacuum */
 		if (ActiveSnapshotSet())
@@ -501,9 +502,9 @@ vacuum(List *relations, VacuumParams *params,
 	{
 		ListCell   *cur;
 
-		VacuumUpdateCosts();
 		in_vacuum = true;
-		VacuumCostActive = (VacuumCostDelay > 0);
+		VacuumFailsafeActive = false;
+		VacuumUpdateCosts();
 		VacuumCostBalance = 0;
 		VacuumPageHit = 0;
 		VacuumPageMiss = 0;
@@ -539,7 +540,7 @@ vacuum(List *relations, VacuumParams *params,
 				}

 				if (use_own_xacts)
 				{
@@ -562,6 +563,9 @@ vacuum(List *relations, VacuumParams *params,
 	{
 		in_vacuum = false;
 		VacuumCostActive = false;
+		VacuumFailsafeActive = false;
+		VacuumCostBalance = 0;
+		analyze_in_outer_xact = false;
 	}
 	PG_END_TRY();
 
@@ -2227,7 +2231,29 @@ vacuum_delay_point(void)
 	/* Always check for interrupts */
 	CHECK_FOR_INTERRUPTS();
 
+		(!VacuumCostActive && !ConfigReloadPending))
+	 * file if it is in an outer transaction, as we currently only allow
+		VacuumUpdateCosts();
+	if (!VacuumCostActive)
 		return;
 
 	/*
diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c
index 4c3c93b2fd..36963090e8 100644
--- a/src/backend/commands/vacuumparallel.c
+++ b/src/backend/commands/vacuumparallel.c
@@ -991,9 +991,8 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
 
 	/* Set cost-based vacuum delay */
 	VacuumFailsafeActive = false;
-	VacuumUpdateCosts();
-	VacuumCostActive = (VacuumCostDelay > 0);
 	VacuumCostBalance = 0;
+	VacuumUpdateCosts();
 	VacuumPageHit = 0;
 	VacuumPageMiss = 0;
 	VacuumPageDirty = 0;
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index ac54ed4546..e7833cd49e 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -1797,6 +1797,24 @@ VacuumUpdateCosts(void)
 		VacuumCostLimit = vacuum_cost_limit;
 		VacuumCostDelay = vacuum_cost_delay;
 	}
+
+	/*
+	* If configuration changes are allowed to impact VacuumCostActive,
+	* make sure it is updated.
+	*/
+	if (VacuumFailsafeActive)
+	{
+		Assert(!VacuumCostActive);
+		return;
+	}
+
+	if (VacuumCostDelay > 0)
+		VacuumCostActive = true;
+	else
+	{
+		VacuumCostActive = false;
+		VacuumCostBalance = 0;
+	}
 }

-- 
2.37.2

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

Reviewed-by: Masahiko Sawada <[email-masked]>
Reviewed-by: Kyotaro Horiguchi <[email-masked]>
Reviewed-by: Daniel Gustafsson <[email-masked]>
Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7%2BDXhrjDA0gw%40mail.gmail.com
---
 src/backend/commands/vacuum.c       |  17 +-
 src/backend/postmaster/autovacuum.c | 235 ++++++++++++++--------------
 src/include/commands/vacuum.h       |   1 +
 3 files changed, 134 insertions(+), 119 deletions(-)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index a288c402a9..774cc5e2b7 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -2237,10 +2237,10 @@ 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 we currently only allow
-	 * configuration reload when in top-level statements.
+	 * [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 we
+	 * currently only allow configuration reload when in top-level statements.
 	 */
 	if (ConfigReloadPending && !analyze_in_outer_xact)
 	{
@@ -2286,7 +2286,14 @@ vacuum_delay_point(void)
 
 		VacuumCostBalance = 0;
 
-		VacuumUpdateCosts();
+		/*
 
 		/* 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 e7833cd49e..1b5b749371 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;

@@ -189,8 +192,8 @@ typedef struct autovac_table
 {
 	Oid			at_relid;
 	VacuumParams at_params;
@@ -209,7 +212,7 @@ typedef struct autovac_table
@@ -223,11 +226,8 @@ typedef struct WorkerInfoData
 
@@ -273,6 +273,8 @@ typedef struct AutoVacuumWorkItem
@@ -286,6 +288,7 @@ typedef struct
 
@@ -319,7 +322,7 @@ static void launch_worker(TimestampTz now);
 
@@ -670,7 +673,7 @@ AutoVacLauncherMain(int argc, char *argv[])
 
@@ -820,8 +823,8 @@ HandleAutoVacLauncherInterrupts(void)
 			AutoVacLauncherShutdown();

@@ -1755,10 +1758,7 @@ FreeWorkerInfo(int code, Datum arg)
@@ -1788,14 +1788,21 @@ VacuumUpdateCosts(void)
 
 	if (am_autovacuum_worker)
 	{
-		VacuumCostLimit = MyWorkerInfo->wi_cost_limit;
-		VacuumCostDelay = MyWorkerInfo->wi_cost_delay;
+		if (av_relopt_cost_delay >= 0)
+			VacuumCostDelay = av_relopt_cost_delay;
+		else if (autovacuum_vac_cost_delay >= 0)
+			VacuumCostDelay = autovacuum_vac_cost_delay;
+		else
+			/* fall back to vacuum_cost_delay */
+			VacuumCostDelay = vacuum_cost_delay;
+
+		AutoVacuumUpdateLimit();
 	}
 	else
 	{
 		/* Must be explicit VACUUM or ANALYZE */
-		VacuumCostLimit = vacuum_cost_limit;
 		VacuumCostDelay = vacuum_cost_delay;
+		VacuumCostLimit = vacuum_cost_limit;
 	}
 
 	/*
@@ -1819,85 +1826,82 @@ VacuumUpdateCosts(void)

 /*
- * autovac_balance_cost
- *		Recalculate the cost limit setting for each active worker.
- *
- * Caller must hold the AutovacuumLock in exclusive mode.
- */
-static void
-autovac_balance_cost(void)
+* 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 must also call it after a config reload.
+*/
+void
+AutoVacuumUpdateLimit(void)
 {
+	if (!am_autovacuum_worker)
+		return;
+
 	/*
-	 * 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 : vacuum_cost_limit);
-	double		vac_cost_delay = (autovacuum_vac_cost_delay >= 0 ?
-								  autovacuum_vac_cost_delay : vacuum_cost_delay);
-	double		cost_total;
-	double		cost_avail;
-	dlist_iter	iter;
-
-	/* not set? nothing to do */
-	if (vac_cost_limit <= 0 || vac_cost_delay <= 0)
-		return;
 
-	/* calculate the total base cost limit of participating active workers */
-	cost_total = 0.0;
-	dlist_foreach(iter, &AutoVacuumShmem->av_runningWorkers)
+	if (av_relopt_cost_limit > 0)
+		VacuumCostLimit = av_relopt_cost_limit;
+	else
 	{
-		WorkerInfo	worker = dlist_container(WorkerInfoData, wi_links, iter.cur);
+		int			nworkers_for_balance;
+
+		if (autovacuum_vac_cost_limit > 0)
+			VacuumCostLimit = autovacuum_vac_cost_limit;
+		else
+			VacuumCostLimit = vacuum_cost_limit;
+
+		/* Only balance limit if no table options specified */
+		if (pg_atomic_unlocked_test_flag(&MyWorkerInfo->wi_dobalance))
+			return;
+
+		Assert(VacuumCostLimit > 0);
 
+		VacuumCostLimit = Max(VacuumCostLimit / nworkers_for_balance, 1);
 	}
+}

-	/*

 
 /*
@@ -2445,23 +2449,31 @@ do_autovacuum(void)
 			continue;
 		}

 
-		/* do a balance */
 
 		VacuumUpdateCosts();
 
-		/* done */
 
 		/* clean up memory before each iteration */
 		MemoryContextResetAndDeleteChildren(PortalContext);
@@ -2546,10 +2558,10 @@ deleted:
 
 		/*
@@ -2586,6 +2598,7 @@ deleted:
+			VacuumUpdateCosts();
 		}
 
 		LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
@@ -2821,8 +2834,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;
 
 		/*
@@ -2832,20 +2843,6 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
 		 * defaults, autovacuum's own first and plain vacuum second.
 		 */
 
-			: vacuum_cost_delay;
-			: vacuum_cost_limit;
@@ -2901,8 +2898,10 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
@@ -3394,10 +3393,18 @@ 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_flag(&worker[i].wi_dobalance);
+		}
+
+		pg_atomic_init_u32(&AutoVacuumShmem->av_nworkersForBalance, 0);
+
 	}
 	else
 		Assert(found);
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index a62dd2e781..6b286037ca 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -351,6 +351,7 @@ extern IndexBulkDeleteResult *vac_cleanup_one_index(IndexVacuumInfo *ivinfo,
 extern Size vac_max_items_to_alloc_size(int max_items);
 
 /* In postmaster/autovacuum.c */
+extern void AutoVacuumUpdateLimit(void);
 extern void VacuumUpdateCosts(void);
 
 /* in commands/vacuumparallel.c */
-- 
2.37.2

From 71a840acc262a3154905f44743130b9668ec78ab Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Fri, 31 Mar 2023 13:10:26 -0400
Subject: [PATCH v12 2/4] Separate vacuum cost variables from gucs

Vacuum code run both by autovacuum workers and a backend doing
VACUUM/ANALYZE previously used VacuumCostLimit and VacuumCostDelay which
were the global variables for the gucs vacuum_cost_limit and
vacuum_cost_delay. Autovacuum workers needed to override these variables
with their own values, derived from autovacuum_vacuum_cost_limit and
autovacuum_vacuum_cost_delay and worker cost limit balancing logic. This
led to confusing code which, in some cases, both derived and set a new
value of VacuumCostLimit from VacuumCostLimit.

In preparation for refreshing these guc values more often, separate
these variables from the gucs themselves and add a function to update
the global variables using the gucs and existing logic.
---
 src/backend/commands/vacuum.c         | 15 +++++++--
 src/backend/commands/vacuumparallel.c |  1 +
 src/backend/postmaster/autovacuum.c   | 47 +++++++++++++--------------
 src/backend/utils/init/globals.c      |  2 --
 src/backend/utils/misc/guc_tables.c   |  4 +--
 src/include/commands/vacuum.h         |  7 ++++
 src/include/miscadmin.h               |  2 --
 src/include/postmaster/autovacuum.h   |  3 --
 8 files changed, 46 insertions(+), 35 deletions(-)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 0e1dbeec70..2c3afd4ff6 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -71,12 +71,22 @@ int			vacuum_multixact_freeze_min_age;
 int			vacuum_multixact_freeze_table_age;
 int			vacuum_failsafe_age;
 int			vacuum_multixact_failsafe_age;
+double		vacuum_cost_delay;
+int			vacuum_cost_limit;

+/*
+ * Variables for cost-based vacuum delay. The defaults differ between
+ * autovacuum and vacuum. These should be overridden with the appropriate GUC
+ * value in vacuum code.
+ * TODO: should they be initialized to valid or invalid values?
+ */
+int			VacuumCostLimit = 0;
+double		VacuumCostDelay = -1;
 
 /*
  * Variables for cost-based parallel vacuum.  See comments atop
@@ -491,6 +501,7 @@ vacuum(List *relations, VacuumParams *params,
 	{
 		ListCell   *cur;
 
+		VacuumUpdateCosts();
 		in_vacuum = true;
 		VacuumCostActive = (VacuumCostDelay > 0);
 		VacuumCostBalance = 0;
@@ -2249,8 +2260,7 @@ vacuum_delay_point(void)
 
 		VacuumCostBalance = 0;
 
+		VacuumUpdateCosts();
 
 		/* Might have gotten an interrupt while sleeping */
 		CHECK_FOR_INTERRUPTS();
@@ -2388,6 +2398,7 @@ vac_max_items_to_alloc_size(int max_items)
 	return offsetof(VacDeadItems, items) + sizeof(ItemPointerData) * max_items;
 }
 
+
 /*
  *	vac_tid_reaped() -- is a particular tid deletable?
  *
diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c
index 57188500d0..4c3c93b2fd 100644
--- a/src/backend/commands/vacuumparallel.c
+++ b/src/backend/commands/vacuumparallel.c
@@ -991,6 +991,7 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
 
 	/* Set cost-based vacuum delay */
 	VacuumFailsafeActive = false;
+	VacuumUpdateCosts();
 	VacuumCostActive = (VacuumCostDelay > 0);
 	VacuumCostBalance = 0;
 	VacuumPageHit = 0;
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 585d28148c..ac54ed4546 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -1773,20 +1773,33 @@ 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 vacuum cost-based delay-related parameters for autovacuum workers and
+ * backends executing VACUUM or ANALYZE using the value of relevant gucs and
+ * global state. This must be called during setup for vacuum and after every
+ * config reload to ensure up-to-date values.
  */
 void
-AutoVacuumUpdateDelay(void)
+VacuumUpdateCosts(void)
 {
-	if (MyWorkerInfo)
+	if (am_autovacuum_launcher)
+		return;
+
+	if (am_autovacuum_worker)
 	{
-		VacuumCostDelay = MyWorkerInfo->wi_cost_delay;
 		VacuumCostLimit = MyWorkerInfo->wi_cost_limit;
+		VacuumCostDelay = MyWorkerInfo->wi_cost_delay;
+	}
+	else
+	{
+		/* Must be explicit VACUUM or ANALYZE */
+		VacuumCostLimit = vacuum_cost_limit;
+		VacuumCostDelay = vacuum_cost_delay;
 	}
 }
 
+
 /*
  * autovac_balance_cost
  *		Recalculate the cost limit setting for each active worker.
@@ -1805,9 +1818,9 @@ autovac_balance_cost(void)
 	 * zero is not a valid value.
 	 */
 	int			vac_cost_limit = (autovacuum_vac_cost_limit > 0 ?
-								  autovacuum_vac_cost_limit : VacuumCostLimit);
+								  autovacuum_vac_cost_limit : vacuum_cost_limit);
 	double		vac_cost_delay = (autovacuum_vac_cost_delay >= 0 ?
-								  autovacuum_vac_cost_delay : VacuumCostDelay);
+								  autovacuum_vac_cost_delay : vacuum_cost_delay);
 	double		cost_total;
 	double		cost_avail;
 	dlist_iter	iter;
@@ -2312,8 +2325,6 @@ do_autovacuum(void)
 		autovac_table *tab;
 		bool		isshared;
 		bool		skipit;
-		double		stdVacuumCostDelay;
-		int			stdVacuumCostLimit;
 		dlist_iter	iter;
 
 		CHECK_FOR_INTERRUPTS();
@@ -2416,14 +2427,6 @@ do_autovacuum(void)
 			continue;
 		}
 
-		/*
 		/* Must hold AutovacuumLock while mucking with cost balance info */
 
@@ -2437,7 +2440,7 @@ do_autovacuum(void)
 		autovac_balance_cost();
 
 		/* set the active cost parameters from the result of that */
-		AutoVacuumUpdateDelay();
+		VacuumUpdateCosts();
 
 		/* done */
@@ -2534,10 +2537,6 @@ deleted:
 
 	/*
@@ -2820,14 +2819,14 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
 			? avopts->vacuum_cost_delay
 			: (autovacuum_vac_cost_delay >= 0)
 			? autovacuum_vac_cost_delay
-			: VacuumCostDelay;
+			: vacuum_cost_delay;
 
 		/* 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;
+			: vacuum_cost_limit;
 
 		/* -1 in autovac setting means use log_autovacuum_min_duration */
 		log_min_duration = (avopts && avopts->log_min_duration >= 0)
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 1b1d814254..8e5b065e8f 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -142,8 +142,6 @@ int			MaxBackends = 0;
 int			VacuumCostPageHit = 1;	/* GUC parameters for vacuum */
 int			VacuumCostPageMiss = 2;
 int			VacuumCostPageDirty = 20;
-int			VacuumCostLimit = 200;
-double		VacuumCostDelay = 0;
 
 int64		VacuumPageHit = 0;
 int64		VacuumPageMiss = 0;
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 8062589efd..77db1a146c 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2409,7 +2409,7 @@ struct config_int ConfigureNamesInt[] =
 			gettext_noop("Vacuum cost amount available before napping."),
 			NULL
 		},
-		&VacuumCostLimit,
+		&vacuum_cost_limit,
 		200, 1, 10000,
 		NULL, NULL, NULL
 	},
@@ -3701,7 +3701,7 @@ struct config_real ConfigureNamesReal[] =
 			NULL,
 			GUC_UNIT_MS
 		},
-		&VacuumCostDelay,
+		&vacuum_cost_delay,
 		0, 0, 100,
 		NULL, NULL, NULL
 	},
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 7b8ee21788..a62dd2e781 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -300,6 +300,8 @@ extern PGDLLIMPORT int vacuum_multixact_freeze_min_age;
 extern PGDLLIMPORT int vacuum_multixact_freeze_table_age;
 extern PGDLLIMPORT int vacuum_failsafe_age;
 extern PGDLLIMPORT int vacuum_multixact_failsafe_age;
+extern PGDLLIMPORT double vacuum_cost_delay;
+extern PGDLLIMPORT int vacuum_cost_limit;
 
 /* Variables for cost-based parallel vacuum */
 extern PGDLLIMPORT pg_atomic_uint32 *VacuumSharedCostBalance;
@@ -307,6 +309,8 @@ extern PGDLLIMPORT pg_atomic_uint32 *VacuumActiveNWorkers;
 extern PGDLLIMPORT int VacuumCostBalanceLocal;
 
 extern bool VacuumFailsafeActive;
+extern int	VacuumCostLimit;
+extern double VacuumCostDelay;
 
 /* in commands/vacuum.c */
 extern void ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel);
@@ -346,6 +350,9 @@ extern IndexBulkDeleteResult *vac_cleanup_one_index(IndexVacuumInfo *ivinfo,
 													IndexBulkDeleteResult *istat);
 extern Size vac_max_items_to_alloc_size(int max_items);
 
+/* In postmaster/autovacuum.c */
+extern void VacuumUpdateCosts(void);
+
 /* in commands/vacuumparallel.c */
 extern ParallelVacuumState *parallel_vacuum_init(Relation rel, Relation *indrels,
 												 int nindexes, int nrequested_workers,
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 06a86f9ac1..66db1b2c69 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -266,8 +266,6 @@ extern PGDLLIMPORT int max_parallel_maintenance_workers;
 extern PGDLLIMPORT int VacuumCostPageHit;
 extern PGDLLIMPORT int VacuumCostPageMiss;
 extern PGDLLIMPORT int VacuumCostPageDirty;
-extern PGDLLIMPORT int VacuumCostLimit;
-extern PGDLLIMPORT double VacuumCostDelay;
 
 extern PGDLLIMPORT int64 VacuumPageHit;
 extern PGDLLIMPORT int64 VacuumPageMiss;
diff --git a/src/include/postmaster/autovacuum.h b/src/include/postmaster/autovacuum.h
index c140371b51..65afd1ea1e 100644
--- a/src/include/postmaster/autovacuum.h
+++ b/src/include/postmaster/autovacuum.h
@@ -63,9 +63,6 @@ extern int	StartAutoVacWorker(void);
 /* called from postmaster when a worker could not be forked */
 extern void AutoVacWorkerFailed(void);
 
-/* autovacuum cost-delay balancer */
-extern void AutoVacuumUpdateDelay(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: Masahiko Sawada <[email-masked]>
Date: Mon, 3 Apr 2023 11:27:42 +0900

> > I will reimplement this in the next version.

Thank you for updating the patches. Here are comments for 0001, 0002,
and 0003 patches:

 0001:

If we go with the idea of using VacuumCostActive +
VacuumFailsafeActive, we need to make sure that both are cleared at
the end of the vacuum per table. Since the patch clears it only here,
it remains true even after vacuum() if we trigger the failsafe mode
for the last table in the table list.

In addition to that,  to ensure that also in an error case, I think we
need to clear it also in PG_FINALLY() block in vacuum().

---
@@ -306,6 +306,7 @@ extern PGDLLIMPORT pg_atomic_uint32
*VacuumSharedCostBalance;
 extern PGDLLIMPORT pg_atomic_uint32 *VacuumActiveNWorkers;
 extern PGDLLIMPORT int VacuumCostBalanceLocal;

Do we need PGDLLIMPORT for VacuumFailSafeActive?

0002:

@@ -2388,6 +2398,7 @@ vac_max_items_to_alloc_size(int max_items)
         return offsetof(VacDeadItems, items) +
sizeof(ItemPointerData) * max_items;
 }

+
 /*

Unnecessary new line. There are some other unnecessary new lines in this patch.

---

and

Do we need PGDLLIMPORT too?

---

+
 /*

Isn't it better to define VacuumUpdateCosts() in vacuum.c rather than
autovacuum.c as this is now a common code for both vacuum and
autovacuum?

0003:

-                VacuumUpdateCosts();
                 in_vacuum = true;

Hmm, if we initialize VacuumFailsafeActive here, should it be included
in 0001 patch?

---

I agree to update VacuumCostActive in VacuumUpdateCosts(). But if we
do that I think this change should be included in 0002 patch.

---

Since analyze_in_outer_xact is false by default, we reload the config
file in vacuum_delay_point() by default. We need to note that
vacuum_delay_point() could be called via other paths, for example
gin_cleanup_pending_list() and ambulkdelete() called by
validate_index(). So it seems to me that we should do the opposite; we
have another global variable, say vacuum_can_reload_config, which is
false by default, and is set to true only when vacuum() allows it. In
vacuum_delay_point(), we reload the config file iff
(ConfigReloadPending && vacuum_can_reload_config).

Regards,

---

From: Melanie Plageman <[email-masked]>
Date: Mon, 3 Apr 2023 12:40:49 -0400

> Thank you for updating the patches. Here are comments for 0001, 0002,

Thanks for the review!

v13 attached with requested updates.

> need to clear it also in PG_FINALLY() block in vacuum().

So, in 0001, I tried to keep it exactly the same as
LVRelState->failsafe_active except for it being a global. We don't
actually use VacuumFailsafeActive in this commit except in vacuumlazy.c,
which does its own management of the value (it resets it to false at the
top of heap_vacuum_rel()).

In the later commit which references VacuumFailsafeActive outside of
vacuumlazy.c, I had reset it in PG_FINALLY(). I hadn't reset it in the
relation list loop in vacuum(). Autovacuum calls vacuum() for each
relation. However, you are right that for VACUUM with a list of
relations for a table access method other than heap, once set to true,
if the table AM forgets to reset the value to false at the end of
vacuuming the relation, it would stay true.

I've set it to false now at the bottom of the loop through relations in
vacuum().

> Do we need PGDLLIMPORT for VacuumFailSafeActive?

I didn't add one because I thought extensions and other code probably
shouldn't access this variable. I thought PGDLLIMPORT was only needed
for extensions built on windows to access variables.

> Unnecessary new line. There are some other unnecessary new lines in this patch.

Thanks! I think I got them all.

> Do we need PGDLLIMPORT too?

I was on the fence about this. I annotated the new guc variables
vacuum_cost_delay and vacuum_cost_limit with PGDLLIMPORT, but I did not
annotate the variables used in vacuum code (VacuumCostLimit/Delay). I
think whether or not this is the right choice depends on two things:
whether or not my understanding of PGDLLIMPORT is correct and, if it is,
whether or not we want extensions to be able to access
VacuumCostLimit/Delay or if just access to the guc variables is
sufficient/desirable.

> +VacuumUpdateCosts(void

We can't access members of WorkerInfoData from inside vacuum.c

> Hmm, if we initialize VacuumFailsafeActive here, should it be included

See comment above. This is the first patch where we use or reference it
outside of vacuumlazy.c

> do that I think this change should be included in 0002 patch.

I'm a bit hesitant to do this because in 0002 VacuumCostActive cannot
change status while vacuuming a table or even between tables for VACUUM
when a list of relations is specified (except for being disabled by
failsafe mode) Adding it to VacuumUpdateCosts() in 0003 makes it clear
that it could change while vacuuming a table, so we must update it.

I previously had 0002 introduce AutoVacuumUpdateLimit(), which only
updated VacuumCostLimit with wi_cost_limit for autovacuum workers and
then called that in vacuum_delay_point() (instead of
AutoVacuumUpdateDelay() or VacuumUpdateCosts()). I abandoned that idea
in favor of the simplicity of having VacuumUpdateCosts() just update
those variables for everyone, since it could be reused in 0003.

Now, I'm thinking the previous method might be more clear?
Or is what I have okay?

> (ConfigReloadPending && vacuum_can_reload_config).

Wow, great point. Thanks for catching this. I've made the update you
suggested. I also set vacuum_can_reload_config to false in PG_FINALLY()
in vacuum().

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

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 888eb022cc..3e34324645 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -2257,10 +2257,10 @@ vacuum_delay_point(void)
 
 	/*
 	if (ConfigReloadPending && vacuum_can_reload_config)
 	{
@@ -2306,7 +2306,14 @@ vacuum_delay_point(void)
 
 		VacuumCostBalance = 0;
 
-		VacuumUpdateCosts();
+		/*
 
index 7a9738202f..8cb9bee4eb 100644

 

 

@@ -1787,14 +1787,21 @@ VacuumUpdateCosts(void)

 	/*
@@ -1817,85 +1824,82 @@ VacuumUpdateCosts(void)
 }
 
 /*
- * autovac_balance_cost

 
 
-	/*

 
 /*
@@ -2443,23 +2447,31 @@ do_autovacuum(void)
 			continue;
 		}

 
-		/* do a balance */

-		/* done */
 
 		/* clean up memory before each iteration */
 		MemoryContextResetAndDeleteChildren(PortalContext);
@@ -2544,10 +2556,10 @@ deleted:
 
 		/*
@@ -2584,6 +2596,7 @@ deleted:
 
 		LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
@@ -2819,8 +2832,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;
 
 		/*
@@ -2830,20 +2841,6 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
 		 * defaults, autovacuum's own first and plain vacuum second.
 		 */
 
@@ -2899,8 +2896,10 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
@@ -3392,10 +3391,18 @@ AutoVacuumShmemInit(void)
 
+

From 6f40f4be3ae8bab5551a298ee6163f67c06861e9 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Mon, 3 Apr 2023 11:22:18 -0400
Subject: [PATCH v13 2/4] Separate vacuum cost variables from gucs

Reviewed-by: Masahiko Sawada <[email-masked]>
Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7%2BDXhrjDA0gw%40mail.gmail.com
---
 src/backend/commands/vacuum.c         | 16 ++++++++--
 src/backend/commands/vacuumparallel.c |  1 +
 src/backend/postmaster/autovacuum.c   | 45 +++++++++++++--------------
 src/backend/utils/init/globals.c      |  2 --
 src/backend/utils/misc/guc_tables.c   |  4 +--
 src/include/commands/vacuum.h         |  7 +++++
 src/include/miscadmin.h               |  2 --
 src/include/postmaster/autovacuum.h   |  3 --
 8 files changed, 45 insertions(+), 35 deletions(-)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 9724fbce46..96df5e2920 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -71,6 +71,18 @@ int			vacuum_multixact_freeze_min_age;
 int			vacuum_multixact_freeze_table_age;
 int			vacuum_failsafe_age;
 int			vacuum_multixact_failsafe_age;
+double		vacuum_cost_delay;
+int			vacuum_cost_limit;
+
+/*
+ * Variables for cost-based vacuum delay. The defaults differ between
+ * autovacuum and vacuum. These should be overridden with the appropriate GUC
+ * value in vacuum code.
+ * TODO: should VacuumCostLimit and VacuumCostDelay be initialized to valid or
+ * invalid values?
+ */
+int			VacuumCostLimit = 0;
+double		VacuumCostDelay = -1;
 
 /*
  * VacuumFailsafeActive is a defined as a global so that we can determine
@@ -498,6 +510,7 @@ vacuum(List *relations, VacuumParams *params,
 	{
 		ListCell   *cur;
 
+		VacuumUpdateCosts();
 		in_vacuum = true;
 		VacuumCostActive = (VacuumCostDelay > 0);
 		VacuumCostBalance = 0;
@@ -2258,8 +2271,7 @@ vacuum_delay_point(void)
 
 		VacuumCostBalance = 0;

 		/* 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 ff7ed0f561..cf8cf89927 100644
--- a/src/backend/commands/vacuumparallel.c
+++ b/src/backend/commands/vacuumparallel.c
@@ -996,6 +996,7 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
 
 	/* Set cost-based vacuum delay */
 	VacuumFailsafeActive = false;
+	VacuumUpdateCosts();
 	VacuumCostActive = (VacuumCostDelay > 0);
 	VacuumCostBalance = 0;
 	VacuumPageHit = 0;
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 585d28148c..27d0d5f9e2 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -1774,16 +1774,27 @@ FreeWorkerInfo(int code, Datum arg)
 }
 
 /*
 
@@ -1805,9 +1816,9 @@ autovac_balance_cost(void)
@@ -2312,8 +2323,6 @@ do_autovacuum(void)
 
 		CHECK_FOR_INTERRUPTS();
@@ -2416,14 +2425,6 @@ do_autovacuum(void)
 			continue;
 		}
 
-		/*
 
@@ -2437,7 +2438,7 @@ do_autovacuum(void)
 		autovac_balance_cost();

 		/* done */
@@ -2534,10 +2535,6 @@ deleted:
 
 	/*
@@ -2820,14 +2817,14 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
 			? avopts->vacuum_cost_delay
 			: (autovacuum_vac_cost_delay >= 0)
 			? autovacuum_vac_cost_delay
-			: VacuumCostDelay;
+			: vacuum_cost_delay;

 
 int64		VacuumPageHit = 0;

 

 

From fabe03a1e3cca701e35e6394c82e218369cd63a1 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Mon, 3 Apr 2023 12:36:35 -0400
Subject: [PATCH v13 3/4] VACUUM reloads config file more often

Reviewed-by: Masahiko Sawada <[email-masked]>
Reviewed-by: Kyotaro Horiguchi <[email-masked]>
Reviewed-by: Daniel Gustafsson <[email-masked]>
Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7%2BDXhrjDA0gw%40mail.gmail.com
---
 src/backend/commands/vacuum.c         | 41 +++++++++++++++++++++++++--
 src/backend/commands/vacuumparallel.c |  5 ++--
 src/backend/postmaster/autovacuum.c   | 18 ++++++++++++
 3 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 96df5e2920..888eb022cc 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"
@@ -83,6 +84,7 @@ int			vacuum_cost_limit;
  */
 int			VacuumCostLimit = 0;
 double		VacuumCostDelay = -1;
+static bool vacuum_can_reload_config = false;
 
 /*
  * VacuumFailsafeActive is a defined as a global so that we can determine
@@ -354,6 +356,8 @@ vacuum(List *relations, VacuumParams *params,
 	else
 		in_outer_xact = IsInTransactionBlock(isTopLevel);
 
+	vacuum_can_reload_config = !in_outer_xact;
+
 	/*
 	 * Check for and disallow recursive calls.  This could happen when VACUUM
 	 * FULL or ANALYZE calls a hostile index expression that itself calls
@@ -510,9 +514,9 @@ vacuum(List *relations, VacuumParams *params,
 	{
 		ListCell   *cur;
 
-		VacuumUpdateCosts();
 		in_vacuum = true;
-		VacuumCostActive = (VacuumCostDelay > 0);
+		VacuumFailsafeActive = false;
+		VacuumUpdateCosts();
 		VacuumCostBalance = 0;
 		VacuumPageHit = 0;
 		VacuumPageMiss = 0;
@@ -566,12 +570,21 @@ vacuum(List *relations, VacuumParams *params,
 					CommandCounterIncrement();
 				}
 			}
+
+			/*
+			 * Ensure VacuumFailsafeActive has been reset before vacuuming the
+			 * next relation relation.
+			 */
+			VacuumFailsafeActive = false;
 		}
 	}
 	PG_FINALLY();
 	{
 		in_vacuum = false;
 		VacuumCostActive = false;
+		VacuumFailsafeActive = false;
+		VacuumCostBalance = 0;
+		vacuum_can_reload_config = false;
 	}
 	PG_END_TRY();
 
@@ -2238,7 +2251,29 @@ vacuum_delay_point(void)
 	/* Always check for interrupts */
 	CHECK_FOR_INTERRUPTS();
 
+	if (ConfigReloadPending && vacuum_can_reload_config)
 
 	/*
diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c
index cf8cf89927..4bc1c14dff 100644
--- a/src/backend/commands/vacuumparallel.c
+++ b/src/backend/commands/vacuumparallel.c
@@ -995,10 +995,9 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
 												 false);
 
 	/* Set cost-based vacuum delay */
-	VacuumFailsafeActive = false;
-	VacuumUpdateCosts();
-	VacuumCostActive = (VacuumCostDelay > 0);
+	Assert(!VacuumFailsafeActive);
 	VacuumCostBalance = 0;
+	VacuumUpdateCosts();
 	VacuumPageHit = 0;
 	VacuumPageMiss = 0;
 	VacuumPageDirty = 0;
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 27d0d5f9e2..7a9738202f 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -1796,6 +1796,24 @@ VacuumUpdateCosts(void)
 		VacuumCostLimit = vacuum_cost_limit;
 		VacuumCostDelay = vacuum_cost_delay;
 	}
+
+	/*
+	 * If configuration changes are allowed to impact VacuumCostActive, make
+	 * sure it is updated.
+	 */
+	if (VacuumFailsafeActive)
+	{
+		Assert(!VacuumCostActive);
+		return;
+	}
+
+	if (VacuumCostDelay > 0)
+		VacuumCostActive = true;
+	else
+	{
+		VacuumCostActive = false;
+		VacuumCostBalance = 0;
+	}
 }
 
 /*
-- 
2.37.2

From b864281b99ecc58d53e0828a3ed1823609f5a7bc Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Fri, 31 Mar 2023 10:38:39 -0400
Subject: [PATCH v13 1/4] Make vacuum's failsafe_active a global

Reviewed-by: Masahiko Sawada <[email-masked]>
Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7%2BDXhrjDA0gw%40mail.gmail.com
---
 src/backend/access/heap/vacuumlazy.c  | 16 +++++++---------
 src/backend/commands/vacuum.c         |  9 +++++++++
 src/backend/commands/vacuumparallel.c |  1 +
 src/include/commands/vacuum.h         |  1 +
 4 files changed, 18 insertions(+), 9 deletions(-)

index 3e5d3982c7..6d761e6d0e 100644

 	/*
 
 	/*

 		/*
 		 * Abandon use of a buffer access strategy to allow use of all of
@@ -2820,7 +2818,7 @@ should_attempt_truncation(LVRelState *vacrel)
 {
 	BlockNumber possibly_freeable;

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index da85330ef4..9724fbce46 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -72,6 +72,15 @@ int			vacuum_multixact_freeze_table_age;
 int			vacuum_failsafe_age;
 int			vacuum_multixact_failsafe_age;
 
+/*
+ * VacuumFailsafeActive is a defined as a global so that we can determine
+ * whether or not to re-enable cost-based vacuum delay when vacuuming a table.
+ * If failsafe mode has been engaged, we will not re-enable cost-based delay
+ * for the table until after vacuuming has completed, regardless of other
+ * settings.
+ */
+bool		VacuumFailsafeActive = false;
+
 /*
  * Variables for cost-based parallel vacuum.  See comments atop
  * compute_parallel_delay to understand how it works.
diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c
index 2cdbd182b6..ff7ed0f561 100644
--- a/src/backend/commands/vacuumparallel.c
+++ b/src/backend/commands/vacuumparallel.c
@@ -995,6 +995,7 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
 												 false);

---

From: Tom Lane <[email-masked]>
Date: Mon, 03 Apr 2023 14:43:14 -0400

> v13 attached with requested updates.

I'm afraid I'd not been paying any attention to this discussion,
but better late than never.  I'm okay with letting autovacuum
processes reload config files more often than now.  However,
I object to allowing ProcessConfigFile to be called from within
commands in a normal user backend.  The existing semantics are
that user backends respond to SIGHUP only at the start of processing
a user command, and I'm uncomfortable with suddenly deciding that
that can work differently if the command happens to be VACUUM.
It seems unprincipled and perhaps actively unsafe.

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Mon, 3 Apr 2023 12:08:37 -0700

Hi,

> It seems unprincipled and perhaps actively unsafe.

I think it should be ok in commands like VACUUM that already internally start
their own transactions, and thus require to be run outside of a transaction
and at the toplevel. I share your concerns about allowing config reload in
arbitrary places. While we might want to go there, it would require a lot more
analysis.

Greetings,

Andres Freund

---

From: Melanie Plageman <[email-masked]>
Date: Mon, 3 Apr 2023 18:35:07 -0400

> arbitrary places. While we might want to go there, it would require a lot more

As an alternative for your consideration, attached v14 set implements
the config file reload for autovacuum only (in 0003) and then enables it
for VACUUM and ANALYZE not in a nested transaction command (in 0004).

Previously I had the commits in the reverse order for ease of review (to
separate changes to worker limit balancing logic from config reload
code).

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

Allow autovacuum to reload the config file more often so that cost-based
delay parameters can take effect while VACUUMing a relation. Previously
autovacuum workers only reloaded the config file once per relation
vacuumed, so config changes could not take effect until beginning to
vacuum the next table.

Reviewed-by: Masahiko Sawada <[email-masked]>
Reviewed-by: Kyotaro Horiguchi <[email-masked]>
Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7%2BDXhrjDA0gw%40mail.gmail.com
---
 src/backend/commands/vacuum.c       |  44 ++++-
 src/backend/postmaster/autovacuum.c | 253 +++++++++++++++-------------
 src/include/commands/vacuum.h       |   1 +
 3 files changed, 180 insertions(+), 118 deletions(-)

index 96df5e2920..a51a3f78a0 100644
@@ -510,9 +511,9 @@ vacuum(List *relations, VacuumParams *params,
 	{
 		ListCell   *cur;
 
-		VacuumUpdateCosts();
 		in_vacuum = true;
@@ -566,12 +567,20 @@ vacuum(List *relations, VacuumParams *params,
 
@@ -2238,7 +2247,27 @@ vacuum_delay_point(void)
 	/* Always check for interrupts */
 	CHECK_FOR_INTERRUPTS();
 
-	if (!VacuumCostActive || InterruptPending)
+	if (InterruptPending ||
+		(!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 && IsAutoVacuumWorkerProcess())
+	{
+		ConfigReloadPending = false;
+		ProcessConfigFile(PGC_SIGHUP);
+		VacuumUpdateCosts();
+	}
+
+	/*
+	 * If we disabled cost-based delays after reloading the config file,
+	 * return.
+	 */
+	if (!VacuumCostActive)
 		return;
 
 	/*
@@ -2271,7 +2300,14 @@ vacuum_delay_point(void)
 
 		VacuumCostBalance = 0;
 
-		VacuumUpdateCosts();
+		/*
 
index e0c568fdaf..8cb9bee4eb 100644

 

 

@@ -1787,97 +1787,119 @@ VacuumUpdateCosts(void)

 /*
- * autovac_balance_cost

 
 
-	/*

 
 /*
@@ -2425,23 +2447,31 @@ do_autovacuum(void)
 			continue;
 		}

 
-		/* do a balance */

-		/* done */
 
 		/* clean up memory before each iteration */
 		MemoryContextResetAndDeleteChildren(PortalContext);
@@ -2526,10 +2556,10 @@ deleted:
 
 		/*
@@ -2566,6 +2596,7 @@ deleted:
 
 		LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
@@ -2801,8 +2832,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 +2841,6 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
 		 * defaults, autovacuum's own first and plain vacuum second.
 		 */
 
@@ -2881,8 +2896,10 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
@@ -3374,10 +3391,18 @@ AutoVacuumShmemInit(void)
 
+

From 4cd9317cdec9c30e97bee30b194c75455c0afc99 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Mon, 3 Apr 2023 17:48:22 -0400
Subject: [PATCH v14 4/4] VACUUM reloads config file more often

A previous commit allowed autovacuum workers to reload the configuration
file more often. Now, enable this behavior for VACUUM and ANALYZE, when
not nested in a transaction command.

Note that, for now, this only applies to the primary VACUUM stage and
not to parallel vacuum workers vacuuming indexes.
---
 src/backend/commands/vacuum.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index a51a3f78a0..3c426ed501 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -84,6 +84,7 @@ int			vacuum_cost_limit;
  */
 int			VacuumCostLimit = 0;
 double		VacuumCostDelay = -1;
+static bool vacuum_can_reload_config = false;
 
 /*
  * VacuumFailsafeActive is a defined as a global so that we can determine
@@ -355,6 +356,8 @@ vacuum(List *relations, VacuumParams *params,
 	else
 		in_outer_xact = IsInTransactionBlock(isTopLevel);
 
@@ -581,6 +584,7 @@ vacuum(List *relations, VacuumParams *params,
 
@@ -2253,10 +2257,12 @@ vacuum_delay_point(void)
 
 	/*
 	 * 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.
+	 * [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 we
+	 * currently only allow configuration reload when in top-level statements.
 	 */
-	if (ConfigReloadPending && IsAutoVacuumWorkerProcess())
+	if (ConfigReloadPending && vacuum_can_reload_config)
 	{
 		ConfigReloadPending = false;
 		ProcessConfigFile(PGC_SIGHUP);
-- 
2.37.2

From 7ee209bee5c096601ed3ad76ee4ab9ced340e3d3 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Mon, 3 Apr 2023 11:22:18 -0400
Subject: [PATCH v14 2/4] Separate vacuum cost variables from gucs

 src/backend/postmaster/autovacuum.c   | 43 +++++++++++++--------------

 /*
 
+		VacuumUpdateCosts();
 		in_vacuum = true;
 
 		VacuumCostBalance = 0;

 		/* 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 563117a8f6..d346838cfc 100644
--- a/src/backend/commands/vacuumparallel.c
+++ b/src/backend/commands/vacuumparallel.c
@@ -995,8 +995,9 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
 												 false);
 
 	/* Set cost-based vacuum delay */
-	VacuumCostActive = (VacuumCostDelay > 0);
 	VacuumCostBalance = 0;
+	VacuumUpdateCosts();
+	VacuumCostActive = (VacuumCostDelay > 0);
 	VacuumPageHit = 0;
 	VacuumPageMiss = 0;
 	VacuumPageDirty = 0;
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 585d28148c..e0c568fdaf 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -1774,17 +1774,28 @@ FreeWorkerInfo(int code, Datum arg)
 }
 
 /*
 		VacuumCostDelay = MyWorkerInfo->wi_cost_delay;
 
 /*
 
 		CHECK_FOR_INTERRUPTS();
 
-		/*

 
 		/* done */
 
 	/*

 
 int64		VacuumPageHit = 0;

 

 

From 4792ff15f6ab2ce52763dd61a2f374dee948a9cd Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Fri, 31 Mar 2023 10:38:39 -0400
Subject: [PATCH v14 1/4] Make vacuum's failsafe_active a global

 3 files changed, 17 insertions(+), 9 deletions(-)

index 639179aa46..2ba85bd3d6 100644

 	/*
 
 	/*

 		/*

 
+/*

---

From: Masahiko Sawada <[email-masked]>
Date: Tue, 4 Apr 2023 17:26:58 +0900

> I've set it to false now at the bottom of the loop through relations in

Agreed. Probably we can merge 0001 into 0003 but I leave it to
committers. The 0001 patch mostly looks good to me except for one
point:

Looking at the 0003 patch, we set VacuumFailsafeActive to false per table:

+                        /*
+                         * Ensure VacuumFailsafeActive has been reset
before vacuuming the
+                         * next relation relation.
+                         */
+                        VacuumFailsafeActive = false;

Given that we ensure it's reset before vacuuming the next table, do we
need to reset it in heap_vacuum_rel?

(there is a typo; s/relation relation/relation/)

> for extensions built on windows to access variables.

Agreed.

> VacuumCostLimit/Delay or if just access to the guc variables is

I guess it would be better to keep both accessible for backward
compatibility. Extensions are able to access both GUC values and
values that are actually used for vacuum delays (as we used to use the
same variables).

> We can't access members of WorkerInfoData from inside vacuum.c

Oops, you're right.

> that it could change while vacuuming a table, so we must update it.

Agreed.

> Now, I'm thinking the previous method might be more clear?

I'm fine with the current one.

> suggested. I also set vacuum_can_reload_config to false in PG_FINALLY()

Here are some review comments for 0002-0004 patches:

0002:
-        if (MyWorkerInfo)

Isn't it a bit safer to check MyWorkerInfo instead of
am_autovacuum_worker? Also, I don't think there is any reason why we
want to exclude only the autovacuum launcher.

---

How about using the default value of normal backends, 200 and 0?

0003:

In vacuum.c, we use snake case for GUC parameters and camel case for
other global variables, so it seems better to rename it
VacuumCanReloadConfig. Sorry, that's my fault.

0004:

+                if (tab->at_dobalance)

The comment of pg_atomic_test_set_flag() says that it returns false if
the flag has not successfully been set:

 * pg_atomic_test_set_flag - TAS()
 *
 * Returns true if the flag has successfully been set, false otherwise.
 *
 * Acquire (including read barrier) semantics.

But IIUC we don't need to worry about that as only one process updates
the flag, right? It might be a good idea to add some comments why we
don't need to check the return value.

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

I think it's better to keep this kind of log in some form for
debugging. For example, we can show these values of autovacuum workers
in VacuumUpdateCosts().

Regards,

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 4 Apr 2023 15:36:28 +0200

> On 4 Apr 2023, at 00:35, Melanie Plageman <[email-masked]> wrote:
> 

Thinking more on this I'm leaning towards going with allowing more frequent
reloads in autovacuum, and saving the same for VACUUM for more careful study.
The general case is probably fine but I'm not convinced that there aren't error
cases which can present unpleasant scenarios.

Regarding the autovacuum part of this patch I think we are down to the final
details and I think it's doable to finish this in time for 16.

> separate changes to worker limit balancing logic from config reload

A few comments on top of already submitted reviews, will do another pass over
this later today.

This comment should be expanded to document who we expect to inspect this
variable in order to decide on cost-based vacuum.

Moving the failsafe switch into a global context means we face the risk of an
extension changing it independently of the GUCs that control it (or the code
relying on it) such that these are out of sync.  External code messing up
internal state is not new and of course outside of our control, but it's worth
at least considering.  There isn't too much we can do here, but perhaps expand
this comment to include a "do not change this" note?

While I agree with upthread review comments that extensions shoulnd't poke at
this, not decorating it with PGDLLEXPORT adds little protection and only cause
inconsistencies in symbol exports across platforms.  We only explicitly hide
symbols in shared libraries IIRC.

 ...

Same with these, I don't think this is according to our default visibility.
Moreover, I'm not sure it's a good idea to perform this rename.  This will keep
VacuumCostLimit and VacuumCostDelay exported, but change their meaning.  Any
external code referring to these thinking they are backing the GUCs will still
compile, but may be broken in subtle ways.  Is there a reason for not keeping
the current GUC variables and instead add net new ones?

I think the important part is to make sure they are never accessed without
VacuumUpdateCosts having been called first.  I think that's the case here, but
it's not entirely clear.  Do you see a codepath where that could happen?  If
they are initialized to a sentinel value we also need to check for that, so
initializing to the defaults from the corresponding GUCs seems better.

Trivial whitespace error in function comment.

These need a comment IMO, ideally one that explain why they are initialized to
those values.

Is there a scenario where this is expected to fail?  If so I think this should
be handled and not just an Assert.

--
Daniel Gustafsson

---

From: Melanie Plageman <[email-masked]>
Date: Tue, 4 Apr 2023 16:04:52 -0400

On Tue, Apr 4, 2023 at 4:27 AM Masahiko Sawada <[email-masked]> wrote:
> The 0001 patch mostly looks good to me except for one
> point:
>

I've changed the one in heap_vacuum_rel() to an assert.

> (there is a typo; s/relation relation/relation/)

Thanks! fixed.

> am_autovacuum_worker?

Ah, since we access it. I've made the change.

> Also, I don't think there is any reason why we want to exclude only
> the autovacuum launcher.

My rationale is that the launcher is the only other process type which
might reasonably be executing this code besides autovac workers, client
backends doing VACUUM/ANALYZE, and parallel vacuum workers. Is it
confusing to have the launcher have VacuumCostLimt and VacuumCostDelay
set to the guc values for explicit VACUUM and ANALYZE -- even if the
launcher doesn't use these variables?

I've removed the check, because I do agree with you that it may be
unnecessarily confusing in the code.

> How about using the default value of normal backends, 200 and 0?

I've gone with this suggestion

> VacuumCanReloadConfig. Sorry, that's my fault.

I have renamed it.

> don't need to check the return value.

I have added this comment.

> debugging. For example, we can show these values of autovacuum workers

I added a message to do_autovacuum() after calling VacuumUpdateCosts()
in the loop vacuuming each table. That means it will happen once per
table. It's not ideal that I had to move the call to VacuumUpdateCosts()
behind the shared lock in that loop so that we could access the pid and
such in the logging message after updating the cost and delay, but it is
probably okay. Though noone is going to be changing those at this
point, it still seemed better to access them under the lock.

This does mean we won't log anything when we do change the values of
VacuumCostDelay and VacuumCostLimit while vacuuming a table. Is it worth
adding some code to do that in VacuumUpdateCosts() (only when the value
has changed not on every call to VacuumUpdateCosts())? Or perhaps we
could add it in the config reload branch that is already in
vacuum_delay_point()?

On Tue, Apr 4, 2023 at 9:36 AM Daniel Gustafsson <[email-masked]> wrote:

Thanks for the review!

> cases which can present unpleasant scenarios.

In attached v15, I've dropped support for VACUUM and non-nested ANALYZE.
It is like a 5 line change and could be added back at any time.

> this comment to include a "do not change this" note?

I've updated the comment to mention how table AM-agnostic VACUUM code
uses it and to say that table AMs can set it if they want that behavior.

> symbols in shared libraries IIRC.

I've updated this.

> the current GUC variables and instead add net new ones?

When VacuumCostLimit was the same variable in the code and for the GUC
vacuum_cost_limit, everytime we reload the config file, VacuumCostLimit
is overwritten. Autovacuum workers have to overwrite this value with the
appropriate one for themselves given the balancing logic and the value
of autovacuum_vacuum_cost_limit. However, the problem is, because you
can specify -1 for autovacuum_vacuum_cost_limit to indicate it should
fall back to vacuum_cost_limit, we have to reference the value of
VacuumCostLimit when calculating the new autovacuum worker's cost limit
after a config reload.

But, you have to be sure you *only* do this after a config reload when
the value of VacuumCostLimit is fresh and unmodified or you risk
dividing the value of VacuumCostLimit over and over. That means it is
unsafe to call functions updating the cost limit more than once.

This orchestration wasn't as difficult when we only reloaded the config
file once every table. We were careful about it and also kept the
original "base" cost limit around from table_recheck_autovac(). However,
once we started reloading the config file more often, this no longer
works.

By separating the variables modified when the gucs are set and the ones
used the code, we can make sure we always have the original value the
guc was set to in vacuum_cost_limit and autovacuum_vacuum_cost_limit,
whenever we need to reference it.

That being said, perhaps we should document what extensions should do?
Do you think they will want to use the variables backing the gucs or to
be able to overwrite the variables being used in the code?

Oh, also I've annotated these with PGDLLIMPORT too.

> initializing to the defaults from the corresponding GUCs seems better.

I don't see a case where autovacuum could access these without calling
VacuumUpdateCosts() first. I think the other callers of
vacuum_delay_point() are the issue (gist/gin/hash/etc).

It might need a bit more thought.

My concern was that these variables correspond to multiple GUCs each
depending on the backend type, and those backends have different
defaults (e.g. autovac workers default cost delay is different than
client backend doing vacuum cost delay).

However, what I have done in this version is initialize them to the
defaults for a client backend executing VACUUM or ANALYZE, since I am
fairly confident that autovacuum will not use them without calling
VacuumUpdateCosts().

> Trivial whitespace error in function comment.

Fixed.

> These need a comment IMO, ideally one that explain why they are initialized to

I've added a comment.

> be handled and not just an Assert.

No, this isn't expected to happen because an autovacuum worker would
have called autovac_recalculate_workers_for_balance() before calling
VacuumUpdateCosts() (which calls AutoVacuumUpdateLimit()) in
do_autovacuum(). But, if someone were to move around or add a call to
VacuumUpdateCosts() there is a chance it could happen.

- Melanie
From 5b11ac2a9b1ddba24b95c5b2a33d791d99e615ba Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Fri, 31 Mar 2023 10:38:39 -0400
Subject: [PATCH v15 1/3] Make vacuum's failsafe_active a global

Reviewed-by: Masahiko Sawada <[email-masked]>
Reviewed-by: Daniel Gustafsson <[email-masked]>
Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7%2BDXhrjDA0gw%40mail.gmail.com
---
 src/backend/access/heap/vacuumlazy.c | 16 +++++++---------
 src/backend/commands/vacuum.c        | 12 ++++++++++++
 src/include/commands/vacuum.h        |  1 +
 3 files changed, 20 insertions(+), 9 deletions(-)

 
 	/*
 
 	/*

 		/*

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index da85330ef4..cf3abb072c 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -72,6 +72,18 @@ int			vacuum_multixact_freeze_table_age;
 int			vacuum_failsafe_age;
 int			vacuum_multixact_failsafe_age;
 
+/*
+ * VacuumFailsafeActive is a defined as a global so that we can determine
+ * whether or not to re-enable cost-based vacuum delay when vacuuming a table.
+ * If failsafe mode has been engaged, we will not re-enable cost-based delay
+ * for the table until after vacuuming has completed, regardless of other
+ * settings. Only VACUUM code should inspect this variable and only table
+ * access methods should set it. In Table AM-agnostic VACUUM code, this
+ * variable controls whether or not to allow cost-based delays. Table AMs are
+ * free to use it if they desire this behavior.
+ */
+bool		VacuumFailsafeActive = false;
+
 /*
  * Variables for cost-based parallel vacuum.  See comments atop
  * compute_parallel_delay to understand how it works.
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index bdfd96cfec..7219c6ba9c 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -306,6 +306,7 @@ extern PGDLLIMPORT pg_atomic_uint32 *VacuumSharedCostBalance;
 extern PGDLLIMPORT pg_atomic_uint32 *VacuumActiveNWorkers;
 extern PGDLLIMPORT int VacuumCostBalanceLocal;
 
+extern PGDLLIMPORT bool VacuumFailsafeActive;

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

Reviewed-by: Masahiko Sawada <[email-masked]>
Reviewed-by: Kyotaro Horiguchi <[email-masked]>
Reviewed-by: Daniel Gustafsson <[email-masked]>
Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7%2BDXhrjDA0gw%40mail.gmail.com
---
 src/backend/access/heap/vacuumlazy.c  |   2 +-
 src/backend/commands/vacuum.c         |  44 ++++-
 src/backend/commands/vacuumparallel.c |   1 -
 src/backend/postmaster/autovacuum.c   | 266 +++++++++++++++-----------
 src/include/commands/vacuum.h         |   1 +
 5 files changed, 196 insertions(+), 118 deletions(-)

index 2ba85bd3d6..0a9ebd22bd 100644
@@ -389,7 +389,7 @@ heap_vacuum_rel(Relation rel, VacuumParams *params,
index b1fc7a0efc..9964c9ea4d 100644
@@ -512,9 +513,9 @@ vacuum(List *relations, VacuumParams *params,
 	{
 		ListCell   *cur;
 
-		VacuumUpdateCosts();
 		in_vacuum = true;
@@ -568,12 +569,20 @@ vacuum(List *relations, VacuumParams *params,
+			 * next relation.
 
@@ -2240,7 +2249,27 @@ vacuum_delay_point(void)
 	/* Always check for interrupts */
 	CHECK_FOR_INTERRUPTS();

 	/*
@@ -2273,7 +2302,14 @@ vacuum_delay_point(void)
 
 		VacuumCostBalance = 0;
 
-		VacuumUpdateCosts();
+		/*
 
 		/* 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 0b59c922e4..e200d5caf8 100644
--- a/src/backend/commands/vacuumparallel.c
+++ b/src/backend/commands/vacuumparallel.c
@@ -995,7 +995,6 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
 												 false);
 
 	/* Set cost-based vacuum delay */
-	VacuumCostActive = (VacuumCostDelay > 0);
 	VacuumUpdateCosts();
 	VacuumCostBalance = 0;
 	VacuumPageHit = 0;
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index ce7e009576..0c1e4652fb 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -139,6 +139,17 @@ int			Log_autovacuum_min_duration = 600000;
 static bool am_autovacuum_launcher = false;
 static bool am_autovacuum_worker = false;
 
+/*
+ * Variables to save the cost-related table options for the current relation
+ * being vacuumed by this autovacuum worker. Using these, we can ensure we
+ * don't overwrite the values of VacuumCostDelay and VacuumCostLimit after
+ * reloading the configuration file. They are initialized to "invalid" values
+ * to indicate no table options were specified and will be set in
+ * do_autovacuum() after checking the table options in table_recheck_autovac().
+ */
+static double av_relopt_cost_delay = -1;
+static int	av_relopt_cost_limit = 0;
+
 /* Flags set by signal handlers */
 static volatile sig_atomic_t got_SIGUSR2 = false;
 
@@ -189,8 +200,8 @@ typedef struct autovac_table
 {
 	Oid			at_relid;
 	VacuumParams at_params;
@@ -209,7 +220,7 @@ typedef struct autovac_table
@@ -223,11 +234,8 @@ typedef struct WorkerInfoData
 
@@ -273,6 +281,8 @@ typedef struct AutoVacuumWorkItem
@@ -286,6 +296,7 @@ typedef struct
 
@@ -319,7 +330,7 @@ static void launch_worker(TimestampTz now);
 
@@ -670,7 +681,7 @@ AutoVacLauncherMain(int argc, char *argv[])
 
@@ -820,8 +831,8 @@ HandleAutoVacLauncherInterrupts(void)
 			AutoVacLauncherShutdown();

@@ -1755,10 +1766,7 @@ FreeWorkerInfo(int code, Datum arg)
@@ -1784,97 +1792,119 @@ VacuumUpdateCosts(void)
 	if (MyWorkerInfo)
 
 /*
- * autovac_balance_cost
- *		Recalculate the cost limit setting for each active worker.
- *
- * Caller must hold the AutovacuumLock in exclusive mode.
+ * 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 must also call it after a config reload.
  */
-static void
-autovac_balance_cost(void)
+void
+AutoVacuumUpdateLimit(void)
 {
+	if (!MyWorkerInfo)
+		return;
+
 	/*
-	 * 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 : vacuum_cost_limit);
-	double		vac_cost_delay = (autovacuum_vac_cost_delay >= 0 ?
-								  autovacuum_vac_cost_delay : vacuum_cost_delay);
-	double		cost_total;
-	double		cost_avail;
-	dlist_iter	iter;

 
 
-	/*

 
 /*
@@ -2422,22 +2452,39 @@ do_autovacuum(void)
 			continue;
 		}

-		/* 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;
+		/*
+		 * We only expect this worker to ever set the flag, so don't bother
+		 * checking the return value. We shouldn't have to retry.
+		 */
+		if (tab->at_dobalance)
+			pg_atomic_test_set_flag(&MyWorkerInfo->wi_dobalance);
+		else
+			pg_atomic_clear_flag(&MyWorkerInfo->wi_dobalance);
 
-		/* do a balance */

-		/* done */
+		elog(DEBUG2, "VacuumUpdateCosts(pid=%d db=%u, rel=%u, dobalance=%s cost_limit=%d, cost_delay=%g)",
+			 MyWorkerInfo->wi_proc->pid, MyWorkerInfo->wi_dboid, MyWorkerInfo->wi_tableoid,
+			 pg_atomic_unlocked_test_flag(&MyWorkerInfo->wi_dobalance) ? "no" : "yes",
+			 VacuumCostLimit, VacuumCostDelay);
+
 		LWLockRelease(AutovacuumLock);
 
 		/* clean up memory before each iteration */
@@ -2523,10 +2570,10 @@ deleted:
 
 		/*
@@ -2563,6 +2610,7 @@ deleted:
 
 		LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
@@ -2798,8 +2846,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;
 
 		/*
@@ -2809,20 +2855,6 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
 		 * defaults, autovacuum's own first and plain vacuum second.
 		 */
 
@@ -2878,8 +2910,10 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
@@ -3371,10 +3405,18 @@ AutoVacuumShmemInit(void)
 
+
index 17cf58255f..ef938fb692 100644

From 00ddceeb3e565bc88f9691da29e01903aa69cf22 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Mon, 3 Apr 2023 11:22:18 -0400
Subject: [PATCH v15 2/3] Separate vacuum cost variables from gucs

Per suggestion by Kyotaro Horiguchi

Reviewed-by: Masahiko Sawada <[email-masked]>
Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7%2BDXhrjDA0gw%40mail.gmail.com
---
 src/backend/commands/vacuum.c         | 15 +++++++++--
 src/backend/commands/vacuumparallel.c |  1 +
 src/backend/postmaster/autovacuum.c   | 38 +++++++++++----------------
 src/backend/utils/init/globals.c      |  2 --
 src/backend/utils/misc/guc_tables.c   |  4 +--
 src/include/commands/vacuum.h         |  7 +++++
 src/include/miscadmin.h               |  2 --
 src/include/postmaster/autovacuum.h   |  3 ---
 8 files changed, 39 insertions(+), 33 deletions(-)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index cf3abb072c..b1fc7a0efc 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -71,6 +71,17 @@ int			vacuum_multixact_freeze_min_age;
 int			vacuum_multixact_freeze_table_age;
 int			vacuum_failsafe_age;
 int			vacuum_multixact_failsafe_age;
+double		vacuum_cost_delay;
+int			vacuum_cost_limit;
+
+/*
+ * Variables for cost-based vacuum delay. The defaults differ between
+ * autovacuum and vacuum. These should be overridden with the appropriate GUC
+ * value in vacuum code. These are initialized here to the defaults for client
+ * backends executing VACUUM or ANALYZE.
+ */
+int			VacuumCostLimit = 200;
+double		VacuumCostDelay = 0;
 
 /*
  * VacuumFailsafeActive is a defined as a global so that we can determine
@@ -501,6 +512,7 @@ vacuum(List *relations, VacuumParams *params,
 	{
 		ListCell   *cur;
 
+		VacuumUpdateCosts();
 		in_vacuum = true;
 		VacuumCostActive = (VacuumCostDelay > 0);
 		VacuumCostBalance = 0;
@@ -2261,8 +2273,7 @@ vacuum_delay_point(void)
 
 		VacuumCostBalance = 0;

index 563117a8f6..0b59c922e4 100644
 
 	/* Set cost-based vacuum delay */
 	VacuumCostActive = (VacuumCostDelay > 0);
+	VacuumUpdateCosts();
 	VacuumCostBalance = 0;
 	VacuumPageHit = 0;
 	VacuumPageMiss = 0;
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 585d28148c..ce7e009576 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -1774,17 +1774,25 @@ FreeWorkerInfo(int code, Datum arg)
 }
 
 /*
 	if (MyWorkerInfo)
 
 /*
@@ -1805,9 +1813,9 @@ autovac_balance_cost(void)
@@ -2312,8 +2320,6 @@ do_autovacuum(void)
 
 		CHECK_FOR_INTERRUPTS();
@@ -2416,14 +2422,6 @@ do_autovacuum(void)
 			continue;
 		}
 
-		/*
 
@@ -2437,7 +2435,7 @@ do_autovacuum(void)
 		autovac_balance_cost();

 		/* done */
@@ -2534,10 +2532,6 @@ deleted:
 
 	/*
@@ -2820,14 +2814,14 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
 			? avopts->vacuum_cost_delay
 			: (autovacuum_vac_cost_delay >= 0)
 			? autovacuum_vac_cost_delay
-			: VacuumCostDelay;
+			: vacuum_cost_delay;

 
 int64		VacuumPageHit = 0;
index 7219c6ba9c..17cf58255f 100644

 extern PGDLLIMPORT bool VacuumFailsafeActive;
+extern PGDLLIMPORT int	VacuumCostLimit;
+extern PGDLLIMPORT double VacuumCostDelay;

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 5 Apr 2023 15:10:55 +0200

> On 4 Apr 2023, at 22:04, Melanie Plageman <[email-masked]> wrote:
> 

> unnecessarily confusing in the code.

+1

> It is like a 5 line change and could be added back at any time.

I think thats the best option for now.

> be able to overwrite the variables being used in the code?

I think I wasn't clear in my comment, sorry.  I don't have a problem with
introducing a new variable to split the balanced value from the GUC value.
What I don't think we should do is repurpose an exported symbol into doing a
new thing.  In the case at hand I think VacuumCostLimit and VacuumCostDelay
should remain the backing variables for the GUCs, with vacuum_cost_limit and
vacuum_cost_delay carrying the balanced values.  So the inverse of what is in
the patch now.

The risk of these symbols being used in extensions might be very low but on
principle it seems unwise to alter a symbol and risk subtle breakage.

> fairly confident that autovacuum will not use them without calling

Another question along these lines, we only call AutoVacuumUpdateLimit() in
case there is a sleep in vacuum_delay_point():

+       /*

Shouldn't we always call that in case we had a config reload, or am I being
thick?

>> +static int av_relopt_cost_limit = 0;

Sorry, I didn't catch this earlier, shouldn't this be -1 to match the default
value of autovacuum_vacuum_cost_limit?

The "table options" nomenclature is right now only used for FDW foreign table
options, I think we should use "storage parameters" or "relation options" here.

> VacuumUpdateCosts() there is a chance it could happen.

Thinking more on this I'm tempted to recommend that we promote this to an
elog(), mainly due to the latter.  An accidental call to VacuumUpdateCosts()
doesn't seem entirely unlikely to happen

--
Daniel Gustafsson

---

From: Melanie Plageman <[email-masked]>
Date: Wed, 5 Apr 2023 11:29:30 -0400

Thanks all for the reviews.

v16 attached. I put it together rather quickly, so there might be a few
spurious whitespaces or similar. There is one rather annoying pgindent
outlier that I have to figure out what to do about as well.

The remaining functional TODOs that I know of are:

- Resolve what to do about names of GUC and vacuum variables for cost
  limit and cost delay (since it may affect extensions)

- Figure out what to do about the logging message which accesses dboid
  and tableoid (lock/no lock, where to put it, etc)

- I see several places in docs which reference the balancing algorithm
  for autovac workers. I did not read them in great detail, but we may
  want to review them to see if any require updates.

- Consider whether or not the initial two commits should just be
  squashed with the third commit

- Anything else reviewers are still unhappy with

On Wed, Apr 5, 2023 at 1:56 AM Masahiko Sawada <[email-masked]> wrote:
>
> On Wed, Apr 5, 2023 at 5:05 AM Melanie Plageman
> Previously, we used to show the pid in the log since a worker/launcher
> set other workers' delay costs. But now that the worker sets its delay
> costs, we don't need to show the pid in the log. Also, I think it's
> useful for debugging and investigating the system if we log it when
> changing the values. The log I imagined to add was like:
>
> @@ -1801,6 +1801,13 @@ VacuumUpdateCosts(void)
>         AutoVacuumUpdateLimit();
> +       elog(DEBUG2, "autovacuum update costs (db=%u, rel=%u,
> dobalance=%s, cost_limit=%d, cost_delay=%g active=%s failsafe=%s)",
> +            MyWorkerInfo->wi_dboid, MyWorkerInfo->wi_tableoid,
> +            VacuumCostLimit, VacuumCostDelay,
> +            VacuumCostDelay > 0 ? "yes" : "no",
> +            VacuumFailsafeActive ? "yes" : "no");
>     }
>     else
>     {

Makes sense. I've updated the log message to roughly what you suggested.
I also realized I think it does make sense to call it in
VacuumUpdateCosts() -- only for autovacuum workers of course. I've done
this. I haven't taken the lock though and can't decide if I must since
they access dboid and tableoid -- those are not going to change at this
point, but I still don't know if I can access them lock-free...
Perhaps there is a way to condition it on the log level?

If I have to take a lock, then I don't know if we should put these in
VacuumUpdateCosts()...

On Wed, Apr 5, 2023 at 3:16 AM Kyotaro Horiguchi
<[email-masked]> wrote:
> About 0001:
> If I understand this correctly, there seems to be an issue. The
> AM-agnostic VACUUM code is setting it and no table AMs actually do
> that.

No, it is not set in table AM-agnostic VACUUM code. I meant it is
used/read from/inspected in table AM-agnostic VACUUM code. Table AMs can
set it if they want to avoid cost-based delays being re-enabled. It is
only set to true heap-specific code and is initialized to false and
reset in table AM-agnostic code back to false in between each relation
being vacuumed. I updated the comment to reflect this. Let me know if
you think it is clear.

> There is no need to reset VacuumFailsafeActive in the PG_TRY() block.

I think that is true -- since it is initialized to false and reset to
false after vacuuming every relation. However, I am leaning toward
keeping it because I haven't thought through every codepath and
determined if there is ever a way where it could be true here.

> I believe we should prevent unnecessary reloading when
> VacuumFailsafeActive is true.

This is in conflict with two of the other reviewers feedback:
Sawada-san:

> +         * take effect while a table is being vacuumed or analyzed.
> +         */
> 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.

and Daniel in response to this:

> > 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.
>
> I agree with this.

> I'm not entirely sure, but it might be better to name this
> AutoVacuumUpdateCostLimit().

I have made this change.

>                 LWLockAcquire(AutovacuumLock, LW_SHARED);
>
>                 autovac_recalculate_workers_for_balance();
>
> I don't see the need for using atomic here. The code is executed
> infrequently and we already take a lock while counting do_balance
> workers. So sticking with the old locking method (taking LW_EXCLUSIVE
> then set wi_dobalance then do balance) should be fine.

We access wi_dobalance on every call to AutoVacuumUpdateLimit() which is
executed in vacuum_delay_point(). I do not think we can justify take a
shared lock in a function that is called so frequently.

> I think we should use wi_dobalance to decide if we need to do balance
> or not. We don't need to take a lock to do that since only the process
> updates it.

We do do that below in the "else" before balancing. But we for sure
don't need to balance if relopt for cost limit is set. We can save an
access to an atomic variable this way. I think the atomic is a
relatively cheap way of avoiding this whole locking question.

> The comment mentions wi_dobalance, but it doesn't appear here..

The point of the comment is that we don't do anything with wi_dobalance
here. It is explaining why it doesn't appear. The previous comment
mentioned not doing anything with wi_cost_delay and wi_cost_limit which
also didn't appear here.

> principle it seems unwise to alter a symbol and risk subtle breakage.

I totally see what you are saying. The only complication is that all of
the other variables used in vacuum code are the camelcase and the gucs
follow the snake case -- as pointed out in a previous review comment by

> VacuumCanReloadConfig. Sorry, that's my fault.

This is less of a compelling argument than subtle breakage for extension
code, though.

I am, however, wondering if extensions expect to have access to the guc
variable or the global variable -- or both?

Left it as is in this version until we resolve the question.

> Shouldn't we always call that in case we had a config reload, or am I being

We actually also call it from inside VacuumUpdateCosts(), which is
always called in the case of a config reload.

> value of autovacuum_vacuum_cost_limit?

Yea, this is a bit tricky. Initial values of -1 and 0 have the same
effect when we are referencing av_relopt_vacuum_cost_limit in
AutoVacuumUpdateCostLimit(). However, I was trying to initialize both
av_relopt_vacuum_cost_limit and av_relopt_vacuum_cost_delay to "invalid"
values which were not the default for the associated autovacuum gucs,
since initializing av_relopt_cost_delay to the default for
autovacuum_vacuum_cost_delay (2 ms) would cause it to be used even if
storage params were not set for the relation.

I have updated the initial value to -1, as you suggested -- but I don't
know if it is more or less confusing the explain what I just explained
in the comment above it.

> options, I think we should use "storage parameters" or "relation options" here.

I've updated these to "storage parameters" to match the docs. I poked
around looking for other places I referred to them as table options and
tried to fix those as well. I've also changed all relevant variable
names.

> doesn't seem entirely unlikely to happen

Makes sense. I've added a trivial elog ERROR, but I didn't spend quite
enough time thinking about what (if any) other context to include in it.

- Melanie
From 8e87c95522d54fdacd77acf1c5b314968d3c7e68 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Fri, 31 Mar 2023 10:38:39 -0400
Subject: [PATCH v16 1/3] Make vacuum's failsafe_active a global

Reviewed-by: Masahiko Sawada <[email-masked]>
Reviewed-by: Daniel Gustafsson <[email-masked]>
Reviewed-by: Kyotaro Horiguchi <[email-masked]>
Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7%2BDXhrjDA0gw%40mail.gmail.com
---
 src/backend/access/heap/vacuumlazy.c | 16 +++++++---------
 src/backend/commands/vacuum.c        | 15 +++++++++++++++
 src/include/commands/vacuum.h        |  1 +
 3 files changed, 23 insertions(+), 9 deletions(-)

 
 	/*
 
 	/*

 		/*

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index da85330ef4..c74b21fce9 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -72,6 +72,21 @@ int			vacuum_multixact_freeze_table_age;
 int			vacuum_failsafe_age;
 int			vacuum_multixact_failsafe_age;
 
+/*
+ * VacuumFailsafeActive is a defined as a global so that we can determine
+ * whether or not to re-enable cost-based vacuum delay when vacuuming a table.
+ * If failsafe mode has been engaged, we will not re-enable cost-based delay
+ * for the table until after vacuuming has completed, regardless of other
+ * settings.
+ *
+ * Only VACUUM code should inspect this variable and only table access methods
+ * should set it to true. In Table AM-agnostic VACUUM code, this variable is
+ * inspected to determine whether or not to allow cost-based delays. Table AMs
+ * are free to set it if they desire this behavior, but it is false by default
+ * and reset to false in between vacuuming each relation.
+ */
+bool		VacuumFailsafeActive = false;
+
 /*
  * Variables for cost-based parallel vacuum.  See comments atop
  * compute_parallel_delay to understand how it works.
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index bdfd96cfec..7219c6ba9c 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -306,6 +306,7 @@ extern PGDLLIMPORT pg_atomic_uint32 *VacuumSharedCostBalance;
 extern PGDLLIMPORT pg_atomic_uint32 *VacuumActiveNWorkers;
 extern PGDLLIMPORT int VacuumCostBalanceLocal;

From e6f7ceb95600149268ba87c3f62c9f549d9e2ca1 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email-masked]>
Date: Mon, 3 Apr 2023 11:22:18 -0400
Subject: [PATCH v16 2/3] Separate vacuum cost variables from gucs

index c74b21fce9..c842d8f1e9 100644
 
 /*
  * VacuumFailsafeActive is a defined as a global so that we can determine
@@ -504,6 +515,7 @@ vacuum(List *relations, VacuumParams *params,
 	{
 		ListCell   *cur;
 
+		VacuumUpdateCosts();
 		in_vacuum = true;
 		VacuumCostActive = (VacuumCostDelay > 0);
 		VacuumCostBalance = 0;
@@ -2264,8 +2276,7 @@ vacuum_delay_point(void)
 
 		VacuumCostBalance = 0;

 
 
 /*
 
 /*
 
 		CHECK_FOR_INTERRUPTS();
 
-		/*

 
 		/* done */
 
 	/*

 
 int64		VacuumPageHit = 0;
index 7219c6ba9c..d048bb6e0d 100644

 

 

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

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 cost-related storage parameters 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
cost-related storage parameters). 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 storage parameters.

 src/backend/postmaster/autovacuum.c   | 271 +++++++++++++++-----------
 5 files changed, 200 insertions(+), 119 deletions(-)

index c842d8f1e9..37fbbe008c 100644
@@ -515,9 +516,9 @@ vacuum(List *relations, VacuumParams *params,
 	{
 		ListCell   *cur;
 
-		VacuumUpdateCosts();
 		in_vacuum = true;
@@ -571,12 +572,20 @@ vacuum(List *relations, VacuumParams *params,
 
@@ -2243,7 +2252,27 @@ vacuum_delay_point(void)
 	/* Always check for interrupts */
 	CHECK_FOR_INTERRUPTS();

 	/*
@@ -2276,7 +2305,14 @@ vacuum_delay_point(void)
 
 		VacuumCostBalance = 0;
 
-		VacuumUpdateCosts();
+		/*
+		AutoVacuumUpdateCostLimit();

 	/* Set cost-based vacuum delay */
-	VacuumCostActive = (VacuumCostDelay > 0);
 	VacuumUpdateCosts();
 	VacuumCostBalance = 0;
 	VacuumPageHit = 0;
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index ce7e009576..15ad2f3df7 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -139,6 +139,18 @@ int			Log_autovacuum_min_duration = 600000;
 static bool am_autovacuum_launcher = false;
 static bool am_autovacuum_worker = false;
 
+/*
+ * Variables to save the cost-related storage parameters for the current
+ * relation being vacuumed by this autovacuum worker. Using these, we can
+ * ensure we don't overwrite the values of VacuumCostDelay and VacuumCostLimit
+ * after reloading the configuration file. They are initialized to "invalid"
+ * values to indicate no cost-related storage parameters were specified and
+ * will be set in do_autovacuum() after checking the storage parameters in
+ * table_recheck_autovac().
+ */
+static double av_storage_param_cost_delay = -1;
+static int	av_storage_param_cost_limit = -1;
+
 /* Flags set by signal handlers */
 static volatile sig_atomic_t got_SIGUSR2 = false;
 
@@ -189,8 +201,8 @@ typedef struct autovac_table
 {
 	Oid			at_relid;
 	VacuumParams at_params;
-	double		at_vacuum_cost_delay;
-	int			at_vacuum_cost_limit;
+	double		at_storage_param_vac_cost_delay;
+	int			at_storage_param_vac_cost_limit;
 	bool		at_dobalance;
 	bool		at_sharedrel;
 	char	   *at_relname;
@@ -209,7 +221,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
@@ -223,11 +235,8 @@ typedef struct WorkerInfoData
 	Oid			wi_tableoid;
 	PGPROC	   *wi_proc;
 	TimestampTz wi_launchtime;
-	bool		wi_dobalance;
+	pg_atomic_flag wi_dobalance;
 	bool		wi_sharedrel;
-	double		wi_cost_delay;
-	int			wi_cost_limit;
-	int			wi_cost_limit_base;
 } WorkerInfoData;
 
@@ -273,6 +282,8 @@ typedef struct AutoVacuumWorkItem
@@ -286,6 +297,7 @@ typedef struct
 
@@ -319,7 +331,7 @@ static void launch_worker(TimestampTz now);
 
@@ -670,7 +682,7 @@ AutoVacLauncherMain(int argc, char *argv[])
 
@@ -820,8 +832,8 @@ HandleAutoVacLauncherInterrupts(void)
 			AutoVacLauncherShutdown();

 		/* rebuild the list in case the naptime changed */
@@ -1755,10 +1767,7 @@ FreeWorkerInfo(int code, Datum arg)
 		MyWorkerInfo->wi_sharedrel = false;
 		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;
+		pg_atomic_clear_flag(&MyWorkerInfo->wi_dobalance);
 		dlist_push_head(&AutoVacuumShmem->av_freeWorkers,
 						&MyWorkerInfo->wi_links);
 		/* not mine anymore */
@@ -1784,97 +1793,127 @@ VacuumUpdateCosts(void)
 {
 	if (MyWorkerInfo)
 	{
-		VacuumCostDelay = MyWorkerInfo->wi_cost_delay;
-		VacuumCostLimit = MyWorkerInfo->wi_cost_limit;
+		if (av_storage_param_cost_delay >= 0)
+			VacuumCostDelay = av_storage_param_cost_delay;
+		else if (autovacuum_vac_cost_delay >= 0)
+			VacuumCostDelay = autovacuum_vac_cost_delay;
+		else
+			/* fall back to vacuum_cost_delay */
+			VacuumCostDelay = vacuum_cost_delay;
+
+		AutoVacuumUpdateCostLimit();
 	}
 	else
 	{
 		/* Must be explicit VACUUM or ANALYZE */
-		VacuumCostLimit = vacuum_cost_limit;
 		VacuumCostDelay = vacuum_cost_delay;
+		VacuumCostLimit = vacuum_cost_limit;
+	}
+
+	/*
+	 * If configuration changes are allowed to impact VacuumCostActive, make
+	 * sure it is updated.
+	 */
+	if (VacuumFailsafeActive)
+		Assert(!VacuumCostActive);
+	else if (VacuumCostDelay > 0)
+		VacuumCostActive = true;
+	else
+	{
+		VacuumCostActive = false;
+		VacuumCostBalance = 0;
+	}
+
+	if (MyWorkerInfo)
+	{
+		elog(DEBUG2,
+			 "Autovacuum VacuumUpdateCosts(db=%u, rel=%u, dobalance=%s, cost_limit=%d, cost_delay=%g active=%s failsafe=%s)",
+			 MyWorkerInfo->wi_dboid, MyWorkerInfo->wi_tableoid,
+			 pg_atomic_unlocked_test_flag(&MyWorkerInfo->wi_dobalance) ? "no" : "yes",
+			 VacuumCostLimit, VacuumCostDelay,
+			 VacuumCostDelay > 0 ? "yes" : "no",
+			 VacuumFailsafeActive ? "yes" : "no");
 	}
 }
 
 /*
- * autovac_balance_cost
+AutoVacuumUpdateCostLimit(void)
 
-	/* not set? nothing to do */
-	if (vac_cost_limit <= 0 || vac_cost_delay <= 0)
-		return;
-
-	/* calculate the total base cost limit of participating active workers */
-	cost_total = 0.0;
-	dlist_foreach(iter, &AutoVacuumShmem->av_runningWorkers)
+	if (av_storage_param_cost_limit > 0)
+		VacuumCostLimit = av_storage_param_cost_limit;
+	else
 	{
-		WorkerInfo	worker = dlist_container(WorkerInfoData, wi_links, iter.cur);
+		int			nworkers_for_balance;
+
+		if (autovacuum_vac_cost_limit > 0)
+			VacuumCostLimit = autovacuum_vac_cost_limit;
+		else
+			VacuumCostLimit = vacuum_cost_limit;
+
+		/* Only balance limit if no cost-related storage parameters specified */
+		if (pg_atomic_unlocked_test_flag(&MyWorkerInfo->wi_dobalance))
+			return;
 
+		if (nworkers_for_balance <= 0)
+			elog(ERROR, "nworkers_for_balance must be > 0");
 
+ *		Recalculate the number of workers to consider, given cost-related
+ *		storage parameters and the current number of active workers.
 
-	/*

 
 /*
@@ -2422,23 +2461,34 @@ do_autovacuum(void)
 			continue;
 		}
 
-		/* Must hold AutovacuumLock while mucking with cost balance info */
-		LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
+		/*
+		 * Save the cost-related storage parameter values in global variables
+		 * for reference when updating VacuumCostLimit and VacuumCostDelay
+		 * during vacuuming this table.
+		 */
+		av_storage_param_cost_limit = tab->at_storage_param_vac_cost_limit;
+		av_storage_param_cost_delay = tab->at_storage_param_vac_cost_delay;

-		/* do a balance */
 
-		/* set the active cost parameters from the result of that */
+		/*
+		 * We wait until this point to update cost delay and cost limit
+		 * values, even though we reloaded the configuration file above, so
+		 * that we can take into account the cost-related storage parameters.
+		 */
 		VacuumUpdateCosts();
 
-		/* done */
 
 		/* clean up memory before each iteration */
 		MemoryContextResetAndDeleteChildren(PortalContext);
@@ -2523,10 +2573,10 @@ 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, unset wi_dobalance on the assumption that we are more likely
+		 * than not to vacuum a table with no cost-related storage parameters
+		 * 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;
@@ -2563,6 +2613,7 @@ deleted:
 		{
 			ConfigReloadPending = false;
 			ProcessConfigFile(PGC_SIGHUP);
+			VacuumUpdateCosts();
 		}
 
 		LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
@@ -2798,8 +2849,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;
 
 		/*
@@ -2809,20 +2858,6 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
 		 * defaults, autovacuum's own first and plain vacuum second.
 		 */
 
@@ -2878,8 +2913,10 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
+		tab->at_storage_param_vac_cost_limit = avopts ?
+		tab->at_storage_param_vac_cost_delay = avopts ?
@@ -3371,10 +3408,18 @@ AutoVacuumShmemInit(void)
 
+
index d048bb6e0d..38c8bdf0fc 100644
 
 /* In postmaster/autovacuum.c */
+extern void AutoVacuumUpdateCostLimit(void);
 extern void VacuumUpdateCosts(void);

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 5 Apr 2023 17:54:46 +0200

> On 5 Apr 2023, at 17:29, Melanie Plageman <[email-masked]> wrote:
>> 

Fair point.

> This is less of a compelling argument than subtle breakage for extension

How about if we rename the variable into something which also acts at bit as
self documenting why there are two in the first place?  Perhaps
BalancedVacuumCostLimit or something similar (I'm terrible with names)?

> variable or the global variable -- or both?

Extensions have access to all exported symbols, and I think it's not uncommon
for extension authors to expect to have access to at least read GUC variables.

--
Daniel Gustafsson

---

From: Robert Haas <[email-masked]>
Date: Wed, 5 Apr 2023 14:55:50 -0400

> - Anything else reviewers are still unhappy with

I really like having the first couple of patches split out -- it makes
them super-easy to understand. A committer can always choose to squash
at commit time if they want. I kind of wish the patch set were split
up more, for even easier understanding. I don't think that's a thing
to get hung up on, but it's an opinion that I have.

I strongly agree with the goals of the patch set, as I understand
them. Being able to change the config file and SIGHUP the server and
have the new values affect running autovacuum workers seems pretty
huge. It would make it possible to solve problems that currently can
only be solved by using gdb on a production instance, which is not a
fun thing to be doing.

+ /*

I don't quite understand what's going on here. A big reason that I'm
worried about this whole issue in the first place is that sometimes
there's a vacuum going on a giant table and you can't get it to go
fast. You want it to absorb new settings, and to do so quickly. I
realize that this is about the number of workers, not the actual cost
limit, so that makes what I'm about to say less important. But ... is
this often enough? Like, the time before we move onto the next table
could be super long. The time before a new worker is launched should
be ~autovacuum_naptime/autovacuum_max_workers or ~20s with default
settings, so that's not horrible, but I'm kind of struggling to
understand the rationale for this particular choice. Maybe it's fine.

To be honest, I think that the whole system where we divide the cost
limit across the workers is the wrong idea. Does anyone actually like
that behavior? This patch probably shouldn't touch that, just in the
interest of getting something done that is an improvement over where
we are now, but I think this behavior is really counterintuitive.
People expect that they can increase autovacuum_max_workers to get
more vacuuming done, and actually in most cases that does not work.
And if that behavior didn't exist, this patch would also be a whole
lot simpler. Again, I don't think this is something we should try to
address right now under time pressure, but in the future, I think we
should consider ripping this behavior out.

+               if (autovacuum_vac_cost_limit > 0)
+                       VacuumCostLimit = autovacuum_vac_cost_limit;
+               else
+                       VacuumCostLimit = vacuum_cost_limit;
+
+               /* Only balance limit if no cost-related storage
parameters specified */
+               if (pg_atomic_unlocked_test_flag(&MyWorkerInfo->wi_dobalance))
+                       return;
+               Assert(VacuumCostLimit > 0);
+
+               nworkers_for_balance = pg_atomic_read_u32(
+
&AutoVacuumShmem->av_nworkersForBalance);
+
+               /* There is at least 1 autovac worker (this worker). */
+               if (nworkers_for_balance <= 0)
+                       elog(ERROR, "nworkers_for_balance must be > 0");
+
+               VacuumCostLimit = Max(VacuumCostLimit /
nworkers_for_balance, 1);

I think it would be better stylistically to use a temporary variable
here and only assign the final value to VacuumCostLimit.

Daniel: Are you intending to commit this?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 5 Apr 2023 21:03:53 +0200

> On 5 Apr 2023, at 20:55, Robert Haas <[email-masked]> wrote:

> Again, I don't think this is something we should try to

I would not be opposed to that, but I wholeheartedly agree that it's not the
job of this patch (or any patch at this point in the cycle).

> here and only assign the final value to VacuumCostLimit.

I can agree with that.  Another supertiny nitpick on the above is to not end a
single-line comment with a period.

> Daniel: Are you intending to commit this?

Yes, my plan is to get it in before feature freeze.  I notice now that I had
missed setting myself as committer in the CF to signal this intent, sorry about
that.

--
Daniel Gustafsson

---

From: Robert Haas <[email-masked]>
Date: Wed, 5 Apr 2023 15:42:33 -0400

On Wed, Apr 5, 2023 at 3:04 PM Daniel Gustafsson <[email-masked]> wrote:
> Yes, my plan is to get it in before feature freeze.

All right, let's make it happen! I think this is pretty close to ready
to ship, and it would solve a problem that is real, annoying, and
serious.

---

From: Peter Geoghegan <[email-masked]>
Date: Wed, 5 Apr 2023 13:19:39 -0700

> more vacuuming done, and actually in most cases that does not work.

I disagree. Increasing autovacuum_max_workers as a method of
increasing the overall aggressiveness of autovacuum seems like the
wrong idea. I'm sure that users do that at times, but they really
ought to have a better way of getting the same result.

ISTM that autovacuum_max_workers confuses the question of what the
maximum possible number of workers should ever be (in extreme cases)
with the question of how many workers might be a good idea given
present conditions.

> lot simpler.

Probably, but the fact remains that the system level view of things is
mostly what matters. The competition between the amount of vacuuming
that we can afford to do right now and the amount of vacuuming that
we'd ideally be able to do really matters. In fact, I'd argue that the
amount of vacuuming that we'd ideally be able to do isn't a
particularly meaningful concept on its own. It's just too hard to
model what we need to do accurately -- emphasizing what we can afford
to do seems much more promising.

> should consider ripping this behavior out.

-1. The delay stuff might not work as well as it should, but it at
least seems like roughly the right idea. The bigger problem seems to
be everything else -- the way that tuning autovacuum_max_workers kinda
makes sense (it shouldn't be an interesting tunable), and the problems
with the autovacuum.c scheduling being so primitive.

-- 
Peter Geoghegan

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 5 Apr 2023 22:38:17 +0200

> On 5 Apr 2023, at 22:19, Peter Geoghegan <[email-masked]> wrote:

> The bigger problem seems to
> makes sense (it shouldn't be an interesting tunable)

Not to derail this thread, and pre-empt a thread where this can be discussed in
its own context, but isn't that kind of the main problem?  Tuning autovacuum is
really complicated and one of the parameters that I think universally seem to
make sense to users is just autovacuum_max_workers.  I agree that it doesn't do
what most think it should, but a quick skim of the name and docs can probably
lead to a lot of folks trying to use it as hammer.

--
Daniel Gustafsson

---

From: Peter Geoghegan <[email-masked]>
Date: Wed, 5 Apr 2023 13:59:03 -0700

> lead to a lot of folks trying to use it as hammer.

I think that I agree. I think that the difficulty of tuning autovacuum
is the actual real problem. (Or maybe it's just very closely related
to the real problem -- the precise definition doesn't seem important.)

There seems to be a kind of physics envy to some of these things.
False precision. The way that the mechanisms actually work (the
autovacuum scheduling, freeze_min_age, and quite a few other things)
*are* simple. But so are the rules of Conway's game of life, yet
people seem to have a great deal of difficulty predicting how it will
behave in any given situation. Any design that focuses on the
immediate consequences of any particular policy while ignoring second
order effects isn't going to work particularly well. Users ought to be
able to constrain the behavior of autovacuum using settings that
express what they want in high level terms. And VACUUM ought to have
much more freedom around finding the best way to meet those high level
goals over time (e.g., very loose rules about how much we need to
advance relfrozenxid by during any individual VACUUM).

-- 
Peter Geoghegan

---

From: Masahiko Sawada <[email-masked]>
Date: Thu, 6 Apr 2023 15:39:43 +0900

> If I have to take a lock, then I don't know if we should put these in

I think we don't need to acquire a lock there as both values are
updated only by workers reporting this message. Also I agree with
where to put the log but I think the log message should start with
lower cases:

+                elog(DEBUG2,
+                         "Autovacuum VacuumUpdateCosts(db=%u, rel=%u,
dobalance=%s, cost_limit=%d, cost_delay=%g active=%s failsafe=%s)",
+                         MyWorkerInfo->wi_dboid, MyWorkerInfo->wi_tableoid,
+
pg_atomic_unlocked_test_flag(&MyWorkerInfo->wi_dobalance) ? "no" :
"yes",
+                         VacuumCostLimit, VacuumCostDelay,
+                         VacuumCostDelay > 0 ? "yes" : "no",
+                         VacuumFailsafeActive ? "yes" : "no");

Some minor comments on 0003:

+/*
+ * autovac_recalculate_workers_for_balance
+ *             Recalculate the number of workers to consider, given
cost-related
+ *             storage parameters and the current number of active workers.
+ *
+ * Caller must hold the AutovacuumLock in at least shared mode to access
+ * worker->wi_proc.
+ */

Does it make sense to add Assert(LWLockHeldByMe(AutovacuumLock)) at
the beginning of this function?

---

Do we really need to have the autovacuum launcher recalculate
av_nworkersForBalance after reloading the config file? Since the cost
parameters are not used inautovac_recalculate_workers_for_balance()
the comment also needs to be updated.

---
+                /*
+                 * Balance and 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.
+                 */
+                AutoVacuumUpdateCostLimit();

I think the last sentence is not correct. IIUC recalculation of
av_nworkersForBalance is done by the launcher after a worker finished
and by workers before vacuuming each table.

---
It's not a problem of this patch, but IIUC since we don't reset
wi_dobalance after vacuuming each table we use the last value of
wi_dobalance for performing autovacuum items. At end of the loop for
tables in do_autovacuum() we have the following code that explains why
we don't reset wi_dobalance:

        /*
         * Remove my info from shared memory.  We could, but intentionally
         * don't, unset wi_dobalance on the assumption that we are more likely
         * than not to vacuum a table with no cost-related storage parameters
         * 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);

Assuming we agree with that, probably we need to reset it to true
after vacuuming all tables?

0001 and 0002 patches look good to me except for the renaming GUCs
stuff as the discussion is ongoing.

Regards,

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 6 Apr 2023 14:29:34 +0200

> On 6 Apr 2023, at 08:39, Masahiko Sawada <[email-masked]> wrote:

> Also I agree with

In principle I agree, but in this case Autovacuum is a name and should IMO in
userfacing messages start with capital A.

> the beginning of this function?

That's probably not a bad idea.

> the comment also needs to be updated.

If I understand this comment right; there was a discussion upthread that simply
doing it in both launcher and worker simplifies the code with little overhead.
A comment can reflect that choice though.

--
Daniel Gustafsson

---

From: Robert Haas <[email-masked]>
Date: Thu, 6 Apr 2023 13:55:12 -0400

> to the real problem -- the precise definition doesn't seem important.)

I agree, and I think that bad choices around what the parameters do
are a big part of the problem. autovacuum_max_workers is one example
of that, but there are a bunch of others. It's not at all intuitive
that if your database gets really big you either need to raise
autovacuum_vacuum_cost_limit or lower autovacuum_vacuum_cost_delay.
And, it's not intuitive either that raising autovacuum_max_workers
doesn't increase the amount of vacuuming that gets done. In my
experience, it's very common for people to observe that autovacuum is
running constantly, and to figure out that the number of running
workers is equal to autovacuum_max_workers at all times, and to then
conclude that they need more workers. So they raise
autovacuum_max_workers and nothing gets any better. In fact, things
might get *worse*, because the time required to complete vacuuming of
a large table can increase if the available bandwidth is potentially
spread across more workers, and it's very often the time to vacuum the
largest tables that determines whether things hold together adequately
or not.

This kind of stuff drives me absolutely batty. It's impossible to make
every database behavior completely intuitive, but here we have a
parameter that seems like it is exactly the right thing to solve the
problem that the user knows they have, and it actually does nothing on
a good day and causes a regression on a bad one. That's incredibly
poor design.

The way it works at the implementation level is pretty kooky, too. The
available resources are split between the workers, but if any of the
relevant vacuum parameters are set for the table currently being
vacuumed, then that worker gets the full resources configured for that
table, and everyone else divides up the amount that's configured
globally. So if you went and set the cost delay and cost limit for all
of your tables to exactly the same values that are configured
globally, you'd vacuum 3 times faster than if you relied on the
identical global defaults (or N times faster, where N is the value
you've picked for autovacuum_max_workers). If you have one really big
table that requires continuous vacuuming, you could slow down
vacuuming on that table through manual configuration settings and
still end up speeding up vacuuming overall, because the remaining
workers would be dividing the budget implied by the default settings
among N-1 workers instead of N workers. As far as I can see, none of
this is documented, which is perhaps for the best, because IMV it
makes no sense.

I think we need to move more toward a model where VACUUM just keeps
up. Emergency mode is a step in that direction, because the definition
of an emergency is that we're definitely not keeping up, but I think
we need something less Boolean. If the database gets bigger or smaller
or more or less active, autovacuum should somehow just adjust to that,
without so much manual fiddling. I think it's good to have the
possibility of some manual fiddling to handle problematic situations,
but you shouldn't have to do it just because you made a table bigger.