On Fri, Mar 10, 2023 at 6:11 PM Melanie Plageman
<melanieplage...@gmail.com> wrote:
>
> Quotes below are combined from two of Sawada-san's emails.
>
> I've also attached a patch with my suggested current version.
>
> On Thu, Mar 9, 2023 at 10:27 PM Masahiko Sawada <sawada.m...@gmail.com> wrote:
> >
> > On Fri, Mar 10, 2023 at 11:23 AM Melanie Plageman
> > <melanieplage...@gmail.com> wrote:
> > >
> > > On Tue, Mar 7, 2023 at 12:10 AM Masahiko Sawada <sawada.m...@gmail.com>
> > > wrote:
> > > >
> > > > On Mon, Mar 6, 2023 at 5:26 AM Melanie Plageman
> > > > <melanieplage...@gmail.com> wrote:
> > > > >
> > > > > On Thu, Mar 2, 2023 at 6:37 PM Melanie Plageman
> > > > > In this version I've removed wi_cost_delay from WorkerInfoData. There
> > > > > is
> > > > > no synchronization of cost_delay amongst workers, so there is no
> > > > > reason
> > > > > to keep it in shared memory.
> > > > >
> > > > > One consequence of not updating VacuumCostDelay from wi_cost_delay is
> > > > > that we have to have a way to keep track of whether or not autovacuum
> > > > > table options are in use.
> > > > >
> > > > > This patch does this in a cringeworthy way. I added two global
> > > > > variables, one to track whether or not cost delay table options are in
> > > > > use and the other to store the value of the table option cost delay. I
> > > > > didn't want to use a single variable with a special value to indicate
> > > > > that table option cost delay is in use because
> > > > > autovacuum_vacuum_cost_delay already has special values that mean
> > > > > certain things. My code needs a better solution.
> > > >
> > > > While it's true that wi_cost_delay doesn't need to be shared, it seems
> > > > to make the logic somewhat complex. We need to handle cost_delay in a
> > > > different way from other vacuum-related parameters and we need to make
> > > > sure av[_use]_table_option_cost_delay are set properly. Removing
> > > > wi_cost_delay from WorkerInfoData saves 8 bytes shared memory per
> > > > autovacuum worker but it might be worth considering to keep
> > > > wi_cost_delay for simplicity.
> > >
> > > Ah, it turns out we can't really remove wi_cost_delay from WorkerInfo
> > > anyway because the launcher doesn't know anything about table options
> > > and so the workers have to keep an updated wi_cost_delay that the
> > > launcher or other autovac workers who are not vacuuming that table can
> > > read from when calculating the new limit in autovac_balance_cost().
> >
> > IIUC if any of the cost delay parameters has been set individually,
> > the autovacuum worker is excluded from the balance algorithm.
>
> Ah, yes! That's right. So it is not a problem. Then I still think
> removing wi_cost_delay from the worker info makes sense. wi_cost_delay
> is a double and can't easily be accessed atomically the way
> wi_cost_limit can be.
>
> Keeping the cost delay local to the backends also makes it clear that
> cost delay is not something that should be written to by other backends
> or that can differ from worker to worker. Without table options in the
> picture, the cost delay should be the same for any worker who has
> reloaded the config file.
>
> As for the cost limit safe access issue, maybe we can avoid a LWLock
> acquisition for reading wi_cost_limit by using an atomic similar to what
> you suggested here for "did_rebalance".
>
> > > I've added in a shared lock for reading from wi_cost_limit in this
> > > patch. However, AutoVacuumUpdateLimit() is called unconditionally in
> > > vacuum_delay_point(), which is called quite often (per block-ish), so I
> > > was trying to think if there is a way we could avoid having to check
> > > this shared memory variable on every call to vacuum_delay_point().
> > > Rebalances shouldn't happen very often (done by the launcher when a new
> > > worker is launched and by workers between vacuuming tables). Maybe we
> > > can read from it less frequently?
> >
> > Yeah, acquiring the lwlock for every call to vacuum_delay_point()
> > seems to be harmful. One idea would be to have one sig_atomic_t
> > variable in WorkerInfoData and autovac_balance_cost() set it to true
> > after rebalancing the worker's cost-limit. The worker can check it
> > without locking and update its delay parameters if the flag is true.
>
> Instead of having the atomic indicate whether or not someone (launcher
> or another worker) did a rebalance, it would simply store the current
> cost limit. Then the worker can normally access it with a simple read.
>
> My rationale is that if we used an atomic to indicate whether or not we
> did a rebalance ("did_rebalance"), we would have the same cache
> coherency guarantees as if we just used the atomic for the cost limit.
> If we read from the "did_rebalance" variable and missed someone having
> written to it on another core, we still wouldn't get around to checking
> the wi_cost_limit variable in shared memory, so it doesn't matter that
> we bothered to keep it in shared memory and use a lock to access it.
>
> I noticed we don't allow wi_cost_limit to ever be less than 0, so we
> could store wi_cost_limit in an atomic uint32.
>
> I'm not sure if it is okay to do pg_atomic_read_u32() and
> pg_atomic_unlocked_write_u32() or if we need pg_atomic_write_u32() in
> most cases.
>
> I've implemented the atomic cost limit in the attached patch. Though,
> I'm pretty unsure about how I initialized the atomics in
> AutoVacuumShmemInit()...
>
> If the consensus is that it is simply too confusing to take
> wi_cost_delay out of WorkerInfo, we might be able to afford using a
> shared lock to access it because we won't call AutoVacuumUpdateDelay()
> on every invocation of vacuum_delay_point() -- only when we've reloaded
> the config file.
One such implementation is attached.
- Melanie
From f0029f1e410852b41b5562939051ede86235508b Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Fri, 10 Mar 2023 18:33:47 -0500
Subject: [PATCH v4] vacuum reloads config file more often
---
src/backend/commands/vacuum.c | 38 ++++++++++++----
src/backend/postmaster/autovacuum.c | 69 +++++++++++++++++++++++------
src/include/postmaster/autovacuum.h | 2 +
3 files changed, 87 insertions(+), 22 deletions(-)
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 2e12baf8eb..9d5ce846a5 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/proc.h"
@@ -75,6 +76,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;
/*
@@ -313,8 +315,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);
@@ -331,10 +332,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,
@@ -456,7 +457,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;
@@ -474,7 +475,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())
@@ -526,7 +527,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)
{
@@ -549,6 +550,7 @@ vacuum(List *relations, VacuumParams *params,
{
in_vacuum = false;
VacuumCostActive = false;
+ analyze_in_outer_xact = false;
}
PG_END_TRY();
@@ -2238,10 +2240,28 @@ vacuum_delay_point(void)
WAIT_EVENT_VACUUM_DELAY);
ResetLatch(MyLatch);
+ /*
+ * Reload the configuration file if requested. This allows changes to
+ * [autovacuum_]vacuum_cost_limit and [autovacuum_]vacuum_cost_delay
+ * to take effect while a table is being vacuumed or analyzed.
+ */
+ if (ConfigReloadPending && !analyze_in_outer_xact)
+ {
+ ConfigReloadPending = false;
+ ProcessConfigFile(PGC_SIGHUP);
+ AutoVacuumUpdateDelay();
+ }
+
VacuumCostBalance = 0;
- /* update balance values for workers */
- AutoVacuumUpdateDelay();
+ /*
+ * Update balance values for workers. We must always do this in case
+ * the autovacuum launcher has done a rebalance (as it does when
+ * launching a new worker).
+ */
+ AutoVacuumUpdateLimit();
+
+ VacuumCostActive = (VacuumCostDelay > 0);
/* Might have gotten an interrupt while sleeping */
CHECK_FOR_INTERRUPTS();
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index c0e2e00a7e..7a202b2bd9 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -226,7 +226,7 @@ typedef struct WorkerInfoData
bool wi_dobalance;
bool wi_sharedrel;
double wi_cost_delay;
- int wi_cost_limit;
+ pg_atomic_uint32 wi_cost_limit;
int wi_cost_limit_base;
} WorkerInfoData;
@@ -743,6 +743,7 @@ AutoVacLauncherMain(int argc, char *argv[])
worker = AutoVacuumShmem->av_startingWorker;
worker->wi_dboid = InvalidOid;
worker->wi_tableoid = InvalidOid;
+ pg_atomic_write_u32(&MyWorkerInfo->wi_cost_limit, 0);
worker->wi_sharedrel = false;
worker->wi_proc = NULL;
worker->wi_launchtime = 0;
@@ -1757,7 +1758,7 @@ FreeWorkerInfo(int code, Datum arg)
MyWorkerInfo->wi_launchtime = 0;
MyWorkerInfo->wi_dobalance = false;
MyWorkerInfo->wi_cost_delay = 0;
- MyWorkerInfo->wi_cost_limit = 0;
+ pg_atomic_write_u32(&MyWorkerInfo->wi_cost_limit, 0);
MyWorkerInfo->wi_cost_limit_base = 0;
dlist_push_head(&AutoVacuumShmem->av_freeWorkers,
&MyWorkerInfo->wi_links);
@@ -1780,11 +1781,36 @@ FreeWorkerInfo(int code, Datum arg)
void
AutoVacuumUpdateDelay(void)
{
- if (MyWorkerInfo)
+ if (!MyWorkerInfo)
+ return;
+
+ LWLockAcquire(AutovacuumLock, LW_SHARED);
+
+ if (MyWorkerInfo->wi_dobalance)
{
- VacuumCostDelay = MyWorkerInfo->wi_cost_delay;
- VacuumCostLimit = MyWorkerInfo->wi_cost_limit;
+ VacuumCostDelay = autovacuum_vac_cost_delay >= 0 ?
+ autovacuum_vac_cost_delay : VacuumCostDelay;
+
+ MyWorkerInfo->wi_cost_delay = VacuumCostDelay;
}
+ else
+ VacuumCostDelay = MyWorkerInfo->wi_cost_delay;
+
+ LWLockRelease(AutovacuumLock);
+
+}
+
+/*
+ * Helper for vacuum_delay_point() to allow workers to read their
+ * wi_cost_limit.
+ */
+void
+AutoVacuumUpdateLimit(void)
+{
+ if (!MyWorkerInfo)
+ return;
+
+ VacuumCostLimit = pg_atomic_read_u32(&MyWorkerInfo->wi_cost_limit);
}
/*
@@ -1855,16 +1881,15 @@ autovac_balance_cost(void)
* 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);
+ pg_atomic_unlocked_write_u32(&worker->wi_cost_limit,
+ Max(Min(limit, worker->wi_cost_limit_base), 1));
}
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,
+ pg_atomic_read_u32(&worker->wi_cost_limit), worker->wi_cost_limit_base,
worker->wi_cost_delay);
}
}
@@ -2326,6 +2351,13 @@ do_autovacuum(void)
ConfigReloadPending = false;
ProcessConfigFile(PGC_SIGHUP);
+ /*
+ * Autovacuum workers should always update VacuumCostDelay and
+ * VacuumCostLimit in case they were overridden by the reload.
+ */
+ AutoVacuumUpdateDelay();
+ AutoVacuumUpdateLimit();
+
/*
* You might be tempted to bail out if we see autovacuum is now
* disabled. Must resist that temptation -- this might be a
@@ -2424,21 +2456,22 @@ do_autovacuum(void)
stdVacuumCostDelay = VacuumCostDelay;
stdVacuumCostLimit = VacuumCostLimit;
+
/* 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;
+ /* this cannot exceed 10000 anyway */
+ pg_atomic_unlocked_write_u32(&MyWorkerInfo->wi_cost_limit,
+ tab->at_vacuum_cost_limit);
MyWorkerInfo->wi_cost_limit_base = tab->at_vacuum_cost_limit;
+ AutoVacuumUpdateLimit();
/* do a balance */
autovac_balance_cost();
- /* set the active cost parameters from the result of that */
- AutoVacuumUpdateDelay();
-
/* done */
LWLockRelease(AutovacuumLock);
@@ -2569,6 +2602,8 @@ deleted:
{
ConfigReloadPending = false;
ProcessConfigFile(PGC_SIGHUP);
+ AutoVacuumUpdateDelay();
+ AutoVacuumUpdateLimit();
}
LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
@@ -3361,6 +3396,7 @@ AutoVacuumShmemInit(void)
{
WorkerInfo worker;
int i;
+ dlist_iter iter;
Assert(!found);
@@ -3374,10 +3410,17 @@ 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);
+
+ dlist_foreach(iter, &AutoVacuumShmem->av_freeWorkers)
+ pg_atomic_init_u32(
+ &(dlist_container(WorkerInfoData, wi_links, iter.cur))->wi_cost_limit,
+ 0);
+
}
else
Assert(found);
diff --git a/src/include/postmaster/autovacuum.h b/src/include/postmaster/autovacuum.h
index c140371b51..558358911c 100644
--- a/src/include/postmaster/autovacuum.h
+++ b/src/include/postmaster/autovacuum.h
@@ -66,6 +66,8 @@ extern void AutoVacWorkerFailed(void);
/* autovacuum cost-delay balancer */
extern void AutoVacuumUpdateDelay(void);
+extern void AutoVacuumUpdateLimit(void);
+
#ifdef EXEC_BACKEND
extern void AutoVacLauncherMain(int argc, char *argv[]) pg_attribute_noreturn();
extern void AutoVacWorkerMain(int argc, char *argv[]) pg_attribute_noreturn();
--
2.37.2
