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>
 <CAD21AoCBqrzCqXPGjhVHJwJThYW_CmBOpWL2J_6Bb_4qMwcsuw@mail.gmail.com>
 <CAAKRu_YxDh-31zzF9vA=6FfggE8-mQo8bwxCxaFqY=pb8kTTag@mail.gmail.com>
 <CAD21AoC=BRMwUpVDf1tzDa13--dLhuq=c2XQ_KAYA4Qx3cS=kA@mail.gmail.com>
 <CAAKRu_Z4NdHk6EofruFzrasO10bzQYCNuvZ=Rri1wJ6vwoV4-g@mail.gmail.com>
 <CAD21AoAJ6medGOeLo4CLkySSPba7yt9UphD-1Sxx+4LY-ixoqw@mail.gmail.com>
 <28639967-F021-4772-A40C-0E6D348CF941@yesql.se> <CAAKRu_YraQ1mtqRJSqv+rvp0Z3VSWhy19gEkAoYqRkmOFRbYXw@mail.gmail.com>
 <CAD21AoCn3-tAB1HgNXY-_B9yaRES0u2gyifw=ABchNV8FkswZg@mail.gmail.com>
In-Reply-To: <CAD21AoCn3-tAB1HgNXY-_B9yaRES0u2gyifw=ABchNV8FkswZg@mail.gmail.com>
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Fri, 24 Mar 2023 13:27:45 -0400
Message-ID: <CAAKRu_a39kpGO_SxWEGAHMEHFAOACk3wwQJhRBaN+7Ke9ua_qg@mail.gmail.com>
Subject: Re: Should vacuum process config file reload more often
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Daniel Gustafsson <daniel@yesql.se>, PostgreSQL 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_a39kpGO_SxWEGAHMEHFAOACk3wwQJhRBaN%2B7Ke9ua_qg%40mail.gmail.com>
Precedence: bulk

On Fri, Mar 24, 2023 at 1:21=E2=80=AFAM Masahiko Sawada <sawada.mshk@gmail.=
com> wrote:
>
> On Fri, Mar 24, 2023 at 9:27=E2=80=AFAM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> >
> > On Thu, Mar 23, 2023 at 2:09=E2=80=AFAM Masahiko Sawada <sawada.mshk@gm=
ail.com> wrote:
> > > On Sun, Mar 19, 2023 at 7:47=E2=80=AFAM Melanie Plageman <melanieplag=
eman@gmail.com> wrote:
> > > Do we need to calculate the number of workers running with
> > > nworkers_for_balance by iterating over the running worker list? I
> > > guess autovacuum workers can increment/decrement it at the beginning
> > > and end of vacuum.
> >
> > I don't think we can do that because if a worker crashes, we have no wa=
y
> > of knowing if it had incremented or decremented the number, so we can't
> > adjust for it.
>
> What kind of crash are you concerned about? If a worker raises an
> ERROR, we can catch it in PG_CATCH() block. If it's a FATAL, we can do
> that in FreeWorkerInfo(). A PANIC error ends up crashing the entire
> server.

Yes, but what about a worker that segfaults? Since table AMs can define
relation_vacuum(), this seems like a real possibility.

I'll address your other code feedback in the next version.

I realized nworkers_for_balance should be initialized to 0 and not 1 --
1 is misleading since there are often 0 autovac workers. We just never
want to use nworkers_for_balance when it is 0. But, workers put a floor
of 1 on the number when they divide limit/nworkers_for_balance (since
they know there must be at least one worker right now since they are a
worker). I thought about whether or not they should call
autovac_balance_cost() if they find that nworkers_for_balance is 0 when
updating their own limit, but I'm not sure.

> > > I need another few readthroughs to figure out of VacuumFailsafeActive=
 does what
> > > I think it does, and should be doing, but in general I think this is =
a good
> > > idea and a patch in good condition close to being committable.
>
> Another approach would be to make VacuumCostActive a ternary value:
> on, off, and never. When we trigger the failsafe mode we switch it to
> never, meaning that it never becomes active even after reloading the
> config file. A good point is that we don't need to add a new global
> variable, but I'm not sure it's better than the current approach.

Hmm, this is interesting. I don't love the word "never" since it kind of
implies a duration longer than the current table being vacuumed. But we
could find a different word or just document it well. For clarity, we
might want to call it failsafe_mode or something.

I wonder if the primary drawback to converting
LVRelState->failsafe_active to a global VacuumFailsafeActive is just the
general rule of limiting scope to the minimum needed.

- Melanie