Re: Should vacuum process config file reload more often

public inbox for [email protected]  
help / color / mirror / Atom feed

From: Melanie Plageman <[email protected]>
To: Masahiko Sawada <[email protected]>
Cc: Pg Hackers <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Amit Kapila <[email protected]>
Subject: Re: Should vacuum process config file reload more often
Date: Thu, 9 Mar 2023 21:22:53 -0500
Message-ID: <CAAKRu_Zir77=EAQxytN4Bc-P-==1YLC9KiK=kqPricjVzac8ng@mail.gmail.com> (raw)
In-Reply-To: <CAD21AoAB-jtKvYd-QbnhbanoY+-PDCXvrX-JhBUqGFrB6e7eTw@mail.gmail.com>
References: <CAAKRu_ZngzqnEODc7LmS1NH04Kt6Y9huSjz5pp7+DXhrjDA0gw@mail.gmail.com>
	<CAD21AoC=b09pzWNyPYuti0M2DQwy57CGF9UWWb_+v=gdWeNG=A@mail.gmail.com>
	<CAAKRu_bR2VrTUskR24GkXV15SxPaCiHYD=soOuWZcT-rGDcA8Q@mail.gmail.com>
	<CAD21AoB=EC6wzjjqDXKmNEhu_BhGg0g_Gc50yh_zvXqiDC_OSg@mail.gmail.com>
	<CAAKRu_buP5wzsho3qNw5o9_R0pF69FRM5hgCmr-mvXmGXwdA7A@mail.gmail.com>
	<CAAKRu_YWNKs2=mE27C1W8VxPJa1UR+YVuckw5kYzWowTStCHiQ@mail.gmail.com>
	<CAD21AoAB-jtKvYd-QbnhbanoY+-PDCXvrX-JhBUqGFrB6e7eTw@mail.gmail.com>

On Tue, Mar 7, 2023 at 12:10 AM Masahiko Sawada <[email protected]> wrote:
>
> On Mon, Mar 6, 2023 at 5:26 AM Melanie Plageman
> <[email protected]> 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().

However, wi_cost_delay is a double, so if we start updating it on config
reload in vacuum_delay_point(), we definitely need some protection
against torn reads.

The table options can only change when workers start vacuuming a new
table, so maybe there is some way to use this to solve this problem?

> > It is worth mentioning that I think that in master,
> > AutoVacuumUpdateDelay() was incorrectly reading wi_cost_limit and
> > wi_cost_delay from shared memory without holding a lock.
>
> Indeed.
>
> > 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.

Maybe we can do something like this with the table options values?

- Melanie

view thread (18+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Should vacuum process config file reload more often
  In-Reply-To: <CAAKRu_Zir77=EAQxytN4Bc-P-==1YLC9KiK=kqPricjVzac8ng@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox