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 <sawada.m...@gmail.com> wrote:
>
> On Wed, Apr 5, 2023 at 5:05 AM Melanie Plageman
> <melanieplage...@gmail.com> wrote:
> >
> > On Tue, Apr 4, 2023 at 4:27 AM Masahiko Sawada <sawada.m...@gmail.com>
> > wrote:
> > > ---
> > > - 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().
> >
> > 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()?
>
> 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)
> VacuumCostDelay = vacuum_cost_delay;
>
> 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,
> + pg_atomic_unlocked_test_flag(&MyWorkerInfo->wi_dobalance)
> ? "no" : "yes",
> + 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
<horikyota....@gmail.com> wrote:
> About 0001:
>
> + * 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;
>
> 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.
> 0003:
> +
> + /*
> + * Ensure VacuumFailsafeActive has been reset before
> vacuuming the
> + * next relation.
> + */
> + VacuumFailsafeActive = false;
> }
> }
> PG_FINALLY();
> {
> in_vacuum = false;
> VacuumCostActive = false;
> + VacuumFailsafeActive = false;
> + VacuumCostBalance = 0;
>
> 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.
> + /*
> + * 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();
> + }
>
> I believe we should prevent unnecessary reloading when
> VacuumFailsafeActive is true.
This is in conflict with two of the other reviewers feedback:
Sawada-san:
> + * Reload the configuration file if requested. This allows changes to
> + * [autovacuum_]vacuum_cost_limit and [autovacuum_]vacuum_cost_delay
> to
> + * take effect while a table is being vacuumed or analyzed.
> + */
> + if (ConfigReloadPending && !analyze_in_outer_xact)
> + {
> + ConfigReloadPending = false;
> + ProcessConfigFile(PGC_SIGHUP);
> + AutoVacuumUpdateDelay();
> + AutoVacuumUpdateLimit();
> + }
>
> 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.
> + AutoVacuumUpdateLimit();
>
> I'm not entirely sure, but it might be better to name this
> AutoVacuumUpdateCostLimit().
I have made this change.
> + pg_atomic_flag wi_dobalance;
> ...
> + /*
> + * 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);
>
> 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.
> +void
> +AutoVacuumUpdateLimit(void)
> ...
> + if (av_relopt_cost_limit > 0)
> + VacuumCostLimit = av_relopt_cost_limit;
> + else
>
> 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.
> /*
> * 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;
>
> 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.
On Wed, Apr 5, 2023 at 9:10 AM Daniel Gustafsson <dan...@yesql.se> wrote:
>
> > On 4 Apr 2023, at 22:04, Melanie Plageman <melanieplage...@gmail.com> wrote:
> >> +extern int VacuumCostLimit;
> >> +extern double VacuumCostDelay;
> >> ...
> >> -extern PGDLLIMPORT int VacuumCostLimit;
> >> -extern PGDLLIMPORT double VacuumCostDelay;
> >>
> >> 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?
> >
> > 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?
>
> 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.
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
Sawada-san:
> @@ -83,6 +84,7 @@ int vacuum_cost_limit;
> */
> int VacuumCostLimit = 0;
> double VacuumCostDelay = -1;
> +static bool vacuum_can_reload_config = false;
>
> 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.
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.
> > Oh, also I've annotated these with PGDLLIMPORT too.
> >
> >> + * TODO: should VacuumCostLimit and VacuumCostDelay be initialized to
> >> valid or
> >> + * invalid values?
> >> + */
> >> +int VacuumCostLimit = 0;
> >> +double VacuumCostDelay = -1;
> >>
> >> 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.
> >
> > 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().
>
> Another question along these lines, we only call AutoVacuumUpdateLimit() in
> case there is a sleep in vacuum_delay_point():
>
> + /*
> + * 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.
> + */
> + AutoVacuumUpdateLimit();
>
> Shouldn't we always call that in case we had a config reload, or am I being
> thick?
We actually also call it from inside VacuumUpdateCosts(), which is
always called in the case of a config reload.
> >> +static double av_relopt_cost_delay = -1;
> >> +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?
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.
> >> These need a comment IMO, ideally one that explain why they are
> >> initialized to
> >> those values.
> >
> > I've added a comment.
>
> + * Variables to save the cost-related table options for the current relation
>
> 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.
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.
> >> + /* There is at least 1 autovac worker (this worker). */
> >> + Assert(nworkers_for_balance > 0);
> >>
> >> Is there a scenario where this is expected to fail? If so I think this
> >> should
> >> 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.
>
> 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
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 <melanieplage...@gmail.com>
Date: Fri, 31 Mar 2023 10:38:39 -0400
Subject: [PATCH v16 1/3] 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.
Reviewed-by: Masahiko Sawada <sawada.m...@gmail.com>
Reviewed-by: Daniel Gustafsson <dan...@yesql.se>
Reviewed-by: Kyotaro Horiguchi <horikyota....@gmail.com>
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/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 639179aa46..2ba85bd3d6 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;
/*
* 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;
- 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 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;
+extern PGDLLIMPORT bool VacuumFailsafeActive;
/* in commands/vacuum.c */
extern void ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel);
--
2.37.2
From e6f7ceb95600149268ba87c3f62c9f549d9e2ca1 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Mon, 3 Apr 2023 11:22:18 -0400
Subject: [PATCH v16 2/3] 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.
Per suggestion by Kyotaro Horiguchi
Reviewed-by: Masahiko Sawada <sawada.m...@gmail.com>
Reviewed-by: Daniel Gustafsson <dan...@yesql.se>
Reviewed-by: Kyotaro Horiguchi <horikyota....@gmail.com>
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 c74b21fce9..c842d8f1e9 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
@@ -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;
- /* update balance values for workers */
- AutoVacuumUpdateDelay();
+ 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 563117a8f6..0b59c922e4 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 */
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)
}
/*
- * 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)
{
VacuumCostDelay = MyWorkerInfo->wi_cost_delay;
VacuumCostLimit = MyWorkerInfo->wi_cost_limit;
}
+ else
+ {
+ /* Must be explicit VACUUM or ANALYZE */
+ VacuumCostLimit = vacuum_cost_limit;
+ VacuumCostDelay = vacuum_cost_delay;
+ }
}
/*
@@ -1805,9 +1813,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 +2320,6 @@ do_autovacuum(void)
autovac_table *tab;
bool isshared;
bool skipit;
- double stdVacuumCostDelay;
- int stdVacuumCostLimit;
dlist_iter iter;
CHECK_FOR_INTERRUPTS();
@@ -2416,14 +2422,6 @@ do_autovacuum(void)
continue;
}
- /*
- * Remember the prevailing values of the vacuum cost GUCs. We have to
- * restore these at the bottom of the loop, else we'll compute wrong
- * values in the next iteration of autovac_balance_cost().
- */
- stdVacuumCostDelay = VacuumCostDelay;
- stdVacuumCostLimit = VacuumCostLimit;
-
/* Must hold AutovacuumLock while mucking with cost balance info */
LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
@@ -2437,7 +2435,7 @@ do_autovacuum(void)
autovac_balance_cost();
/* set the active cost parameters from the result of that */
- AutoVacuumUpdateDelay();
+ VacuumUpdateCosts();
/* done */
LWLockRelease(AutovacuumLock);
@@ -2534,10 +2532,6 @@ deleted:
MyWorkerInfo->wi_tableoid = InvalidOid;
MyWorkerInfo->wi_sharedrel = false;
LWLockRelease(AutovacuumScheduleLock);
-
- /* restore vacuum cost GUCs for the next iteration */
- VacuumCostDelay = stdVacuumCostDelay;
- VacuumCostLimit = stdVacuumCostLimit;
}
/*
@@ -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;
/* 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 7219c6ba9c..d048bb6e0d 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 PGDLLIMPORT bool VacuumFailsafeActive;
+extern PGDLLIMPORT int VacuumCostLimit;
+extern PGDLLIMPORT 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 e2a52318a35fbe7236b675f5a7c210d02568aadf Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Sat, 25 Mar 2023 14:14:55 -0400
Subject: [PATCH v16 3/3] 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.
Now, check if a reload is pending roughly once per block, when checking
if we need to delay.
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 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.
Reviewed-by: Masahiko Sawada <sawada.m...@gmail.com>
Reviewed-by: Daniel Gustafsson <dan...@yesql.se>
Reviewed-by: Kyotaro Horiguchi <horikyota....@gmail.com>
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 | 271 +++++++++++++++-----------
src/include/commands/vacuum.h | 1 +
5 files changed, 200 insertions(+), 119 deletions(-)
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 2ba85bd3d6..0a9ebd22bd 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -389,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);
- VacuumFailsafeActive = false;
+ Assert(!VacuumFailsafeActive);
vacrel->consider_bypass_optimization = true;
vacrel->do_index_vacuuming = true;
vacrel->do_index_cleanup = true;
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index c842d8f1e9..37fbbe008c 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"
@@ -515,9 +516,9 @@ vacuum(List *relations, VacuumParams *params,
{
ListCell *cur;
- VacuumUpdateCosts();
in_vacuum = true;
- VacuumCostActive = (VacuumCostDelay > 0);
+ VacuumFailsafeActive = false;
+ VacuumUpdateCosts();
VacuumCostBalance = 0;
VacuumPageHit = 0;
VacuumPageMiss = 0;
@@ -571,12 +572,20 @@ vacuum(List *relations, VacuumParams *params,
CommandCounterIncrement();
}
}
+
+ /*
+ * Ensure VacuumFailsafeActive has been reset before vacuuming the
+ * next relation.
+ */
+ VacuumFailsafeActive = false;
}
}
PG_FINALLY();
{
in_vacuum = false;
VacuumCostActive = false;
+ VacuumFailsafeActive = false;
+ VacuumCostBalance = 0;
}
PG_END_TRY();
@@ -2243,7 +2252,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;
/*
@@ -2276,7 +2305,14 @@ vacuum_delay_point(void)
VacuumCostBalance = 0;
- VacuumUpdateCosts();
+ /*
+ * 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();
/* 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..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;
typedef struct WorkerInfoData *WorkerInfo;
@@ -273,6 +282,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 +297,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 +331,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 +682,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 +832,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 +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
- * 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
+AutoVacuumUpdateCostLimit(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;
- /* 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 (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;
+ 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);
}
+}
- /* there are no cost limits -- nothing to do */
- if (cost_total <= 0)
- return;
+/*
+ * 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.
+ */
+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);
- /*
- * 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;
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);
}
/*
@@ -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;
- /* 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 */
- 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 storage parameters.
+ */
VacuumUpdateCosts();
- /* done */
- LWLockRelease(AutovacuumLock);
/* 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.
*/
- /* -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
- : 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
- : vacuum_cost_limit;
-
/* -1 in autovac setting means use log_autovacuum_min_duration */
log_min_duration = (avopts && avopts->log_min_duration >= 0)
? avopts->log_min_duration
@@ -2878,8 +2913,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_storage_param_vac_cost_limit = avopts ?
+ avopts->vacuum_cost_limit : 0;
+ tab->at_storage_param_vac_cost_delay = avopts ?
+ avopts->vacuum_cost_delay : -1;
tab->at_relname = NULL;
tab->at_nspname = NULL;
tab->at_datname = NULL;
@@ -3371,10 +3408,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/commands/vacuum.h b/src/include/commands/vacuum.h
index d048bb6e0d..38c8bdf0fc 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 AutoVacuumUpdateCostLimit(void);
extern void VacuumUpdateCosts(void);
/* in commands/vacuumparallel.c */
--
2.37.2
