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>
In-Reply-To: <CAD21AoAB-jtKvYd-QbnhbanoY+-PDCXvrX-JhBUqGFrB6e7eTw@mail.gmail.com>
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Thu, 9 Mar 2023 21:22:53 -0500
Message-ID: <CAAKRu_Zir77=EAQxytN4Bc-P-==1YLC9KiK=kqPricjVzac8ng@mail.gmail.com>
Subject: Re: Should vacuum process config file reload more often
To: Masahiko Sawada <sawada.mshk@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/CAAKRu_Zir77%3DEAQxytN4Bc-P-%3D%3D1YLC9KiK%3DkqPricjVzac8ng%40mail.gmail.com>
Precedence: bulk

On Tue, Mar 7, 2023 at 12:10=E2=80=AFAM Masahiko Sawada <sawada.mshk@gmail.=
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 i=
s
> > 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