Content-Type: text/plain;
	charset=us-ascii
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: <CAAKRu_bJ2DJxYoMkyMo9UpoX74LY5d2Jir86QWFUftrGKWP5Bw@mail.gmail.com>
Date: Wed, 5 Apr 2023 17:54:46 +0200
Cc: Andres Freund <andres@anarazel.de>,
 Tom Lane <tgl@sss.pgh.pa.us>,
 Masahiko Sawada <sawada.mshk@gmail.com>,
 Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
 Amit Kapila <amit.kapila16@gmail.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <2B12DDCC-63C4-49FD-B485-3479E543E2C5@yesql.se>
References: <20230329.132155.629765142788133576.horikyota.ntt@gmail.com>
 <20230329.173456.1185961934810139447.horikyota.ntt@gmail.com>
 <CAAKRu_Yg4-kWH_EN7wtpm3z9B+H27UTmOxzzYv1Vg=YdZ6v_rw@mail.gmail.com>
 <CAD21AoCGdxYAJHmE_nXw_MFw1+Qtn3vcThRu5FiadukgQnpWeQ@mail.gmail.com>
 <935191FB-083B-4060-89FC-466F61FAA391@yesql.se>
 <CAAKRu_aoa0YT36q2n3xa9MZYdepeH4Jyf2EVJj=7YdC_1c974Q@mail.gmail.com>
 <CAAKRu_bSsChKsyEy1ZV2XJQ22FoDdA56efjNr9==m2TO94557w@mail.gmail.com>
 <CAD21AoDi4i4tj1Cf9BG0q2iyVhHisQ2xxVBZajEqExX6t6_b3g@mail.gmail.com>
 <CAAKRu_bkg+0WAGTxsy0HP75RzFbPwqz3ctYq1kThjFhSL69upw@mail.gmail.com>
 <1252075.1680547394@sss.pgh.pa.us>
 <20230403190837.qubpnwugfe2k2g46@awork3.anarazel.de>
 <CAAKRu_ap7nhmqtKQ1TRo3LzHG3FK=YpGO=O0019M3BbN7ysm3A@mail.gmail.com>
 <7E06F879-7E20-4A6A-862F-CA72CDC9A323@yesql.se>
 <CAAKRu_b1HjGCTsFpUnmwLNS8NeXJ+JnrDLhT1osP+Gq9HCU+Rw@mail.gmail.com>
 <CBF0083E-5893-4B35-9498-142A835D03DC@yesql.se>
 <CAAKRu_bJ2DJxYoMkyMo9UpoX74LY5d2Jir86QWFUftrGKWP5Bw@mail.gmail.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Archived-At: <https://www.postgresql.org/message-id/2B12DDCC-63C4-49FD-B485-3479E543E2C5%40yesql.se>
Precedence: bulk

> On 5 Apr 2023, at 17:29, Melanie Plageman <melanieplageman@gmail.com> =
wrote:
>>=20
>> I think I wasn't clear in my comment, sorry.  I don't have a problem =
with
>> introducing a new variable to split the balanced value from the GUC =
value.
>> What I don't think we should do is repurpose an exported symbol into =
doing a
>> new thing.  In the case at hand I think VacuumCostLimit and =
VacuumCostDelay
>> should remain the backing variables for the GUCs, with =
vacuum_cost_limit and
>> vacuum_cost_delay carrying the balanced values.  So the inverse of =
what is in
>> the patch now.
>>=20
>> The risk of these symbols being used in extensions might be very low =
but on
>> principle it seems unwise to alter a symbol and risk subtle breakage.
>=20
> I totally see what you are saying. The only complication is that all =
of
> the other variables used in vacuum code are the camelcase and the gucs
> follow the snake case -- as pointed out in a previous review comment =
by
> Sawada-san:

Fair point.

>> @@ -83,6 +84,7 @@ int                   vacuum_cost_limit;
>>  */
>> int                    VacuumCostLimit =3D 0;
>> double         VacuumCostDelay =3D -1;
>> +static bool vacuum_can_reload_config =3D false;
>>=20
>> In vacuum.c, we use snake case for GUC parameters and camel case for
>> other global variables, so it seems better to rename it
>> VacuumCanReloadConfig. Sorry, that's my fault.
>=20
> This is less of a compelling argument than subtle breakage for =
extension
> code, though.

How about if we rename the variable into something which also acts at =
bit as
self documenting why there are two in the first place?  Perhaps
BalancedVacuumCostLimit or something similar (I'm terrible with names)?

> I am, however, wondering if extensions expect to have access to the =
guc
> variable or the global variable -- or both?

Extensions have access to all exported symbols, and I think it's not =
uncommon
for extension authors to expect to have access to at least read GUC =
variables.

--
Daniel Gustafsson