Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.120.41.1.2\))
Subject: Re: Should vacuum process config file reload more often
From: Daniel Gustafsson <daniel@yesql.se>
In-Reply-To: <CAD21AoAJ6medGOeLo4CLkySSPba7yt9UphD-1Sxx+4LY-ixoqw@mail.gmail.com>
Date: Thu, 23 Mar 2023 17:24:36 +0100
Cc: Melanie Plageman <melanieplageman@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
 Andres Freund <andres@anarazel.de>,
 Amit Kapila <amit.kapila16@gmail.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <28639967-F021-4772-A40C-0E6D348CF941@yesql.se>
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>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Archived-At: <https://www.postgresql.org/message-id/28639967-F021-4772-A40C-0E6D348CF941%40yesql.se>
Precedence: bulk

> On 23 Mar 2023, at 07:08, Masahiko Sawada <sawada.mshk@gmail.com> =
wrote:
> On Sun, Mar 19, 2023 at 7:47=E2=80=AFAM Melanie Plageman =
<melanieplageman@gmail.com> wrote:

> It makes sense to me that we need to reload the config file even when
> vacuum-delay is disabled. But I think it's not convenient for users
> that we don't reload the configuration file once the failsafe is
> triggered. I think users might want to change some GUCs such as
> log_autovacuum_min_duration.

I agree with this.

>> On an unrelated note, I was wondering if there were any docs anywhere
>> that should be updated to go along with this.
>=20
> The current patch improves the internal mechanism of (re)balancing
> vacuum-cost but doesn't change user-visible behavior. I don't have any
> idea so far that we should update somewhere in the doc.

I had a look as well and can't really spot anywhere where the current =
behavior
is detailed, so there is little to update.  On top of that, I also don't =
think
it's worth adding this to the docs.

>> And, I was wondering if it was worth trying to split up the part that
>> reloads the config file and all of the autovacuum stuff. The =
reloading
>> of the config file by itself won't actually result in autovacuum =
workers
>> having updated cost delays because of them overwriting it with
>> wi_cost_delay, but it will allow VACUUM to have those updated values.
>=20
> It makes sense to me to have changes for overhauling the rebalance
> mechanism in a separate patch.

It would for sure be worth considering,=20

+bool       VacuumFailsafeActive =3D false;
This needs documentation, how it's used and how it relates to =
failsafe_active
in LVRelState (which it might replace(?), but until then).

+   pg_atomic_uint32 nworkers_for_balance;
This needs a short oneline documentation update to the struct comment.


-   double      wi_cost_delay;
-   int         wi_cost_limit;
-   int         wi_cost_limit_base;
This change makes the below comment in do_autovacuum in need of an =
update:
  /*
   * Remove my info from shared memory.  We could, but intentionally.
   * don't, clear wi_cost_limit and friends --- this is on the
   * assumption that we probably have more to do with similar cost
   * settings, so we don't want to give up our share of I/O for a very
   * short interval and thereby thrash the global balance.
   */


+   if (av_table_option_cost_delay >=3D 0)
+       VacuumCostDelay =3D av_table_option_cost_delay;
+   else
+       VacuumCostDelay =3D autovacuum_vac_cost_delay >=3D 0 ?
+           autovacuum_vac_cost_delay : VacuumCostDelay;
While it's a matter of personal preference, I for one would like if we =
reduced
the number of ternary operators in the vacuum code, especially those =
mixed into
if statements.  The vacuum code is full of this already though so this =
isn't
less of an objection (as it follows style) than an observation.


+    * note: in cost_limit, zero also means use value from elsewhere, =
because
+    * zero is not a valid value.
...
+       int         vac_cost_limit =3D autovacuum_vac_cost_limit > 0 ?
+       autovacuum_vac_cost_limit : VacuumCostLimit;
Not mentioning the fact that a magic value in a GUC means it's using the =
value
from another GUC (which is not great IMHO), it seems we are using zero =
as well
as -1 as that magic value here?  (not introduced in this patch.) The =
docs does
AFAICT only specify -1 as that value though.  Am I missing something or =
is the
code and documentation slightly out of sync?

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.

--
Daniel Gustafsson=