MIME-Version: 1.0
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> <CAAKRu_Zir77=EAQxytN4Bc-P-==1YLC9KiK=kqPricjVzac8ng@mail.gmail.com>
In-Reply-To: <CAAKRu_Zir77=EAQxytN4Bc-P-==1YLC9KiK=kqPricjVzac8ng@mail.gmail.com>
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Fri, 10 Mar 2023 12:26:23 +0900
Message-ID: <CAD21AoCBqrzCqXPGjhVHJwJThYW_CmBOpWL2J_6Bb_4qMwcsuw@mail.gmail.com>
Subject: Re: Should vacuum process config file reload more often
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>, Andres Freund <andres@anarazel.de>, 
	Amit Kapila <amit.kapila16@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAD21AoCBqrzCqXPGjhVHJwJThYW_CmBOpWL2J_6Bb_4qMwcsuw%40mail.gmail.com>
Precedence: bulk

On Fri, Mar 10, 2023 at 11:23=E2=80=AFAM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> On Tue, Mar 7, 2023 at 12:10=E2=80=AFAM Masahiko Sawada <sawada.mshk@gmai=
l.com> wrote:
> >
> > On Mon, Mar 6, 2023 at 5:26=E2=80=AFAM Melanie Plageman
> > <melanieplageman@gmail.com> wrote:
> > >
> > > On Thu, Mar 2, 2023 at 6:37=E2=80=AFPM 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 reas=
on
> > > 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 i=
n
> > > 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.

>
> 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 n=
ew
> > > 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?

Since an autovacuum that uses any of table option cost delay
parameters is excluded from the balancing algorithm, the launcher
doesn't need to notify such workers of changes of the cost-limit, no?

Regards,

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