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: <CAAKRu_ap7nhmqtKQ1TRo3LzHG3FK=YpGO=O0019M3BbN7ysm3A@mail.gmail.com>
Date: Tue, 4 Apr 2023 15:36:28 +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: <7E06F879-7E20-4A6A-862F-CA72CDC9A323@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>
To: Melanie Plageman <melanieplageman@gmail.com>
Archived-At: <https://www.postgresql.org/message-id/7E06F879-7E20-4A6A-862F-CA72CDC9A323%40yesql.se>
Precedence: bulk

> On 4 Apr 2023, at 00:35, Melanie Plageman <melanieplageman@gmail.com> =
wrote:
>=20
> On Mon, Apr 3, 2023 at 3:08=E2=80=AFPM Andres Freund =
<andres@anarazel.de> wrote:
>> On 2023-04-03 14:43:14 -0400, Tom Lane wrote:
>>> Melanie Plageman <melanieplageman@gmail.com> writes:
>>>> v13 attached with requested updates.
>>>=20
>>> I'm afraid I'd not been paying any attention to this discussion,
>>> but better late than never.  I'm okay with letting autovacuum
>>> processes reload config files more often than now.  However,
>>> I object to allowing ProcessConfigFile to be called from within
>>> commands in a normal user backend.  The existing semantics are
>>> that user backends respond to SIGHUP only at the start of processing
>>> a user command, and I'm uncomfortable with suddenly deciding that
>>> that can work differently if the command happens to be VACUUM.
>>> It seems unprincipled and perhaps actively unsafe.
>>=20
>> I think it should be ok in commands like VACUUM that already =
internally start
>> their own transactions, and thus require to be run outside of a =
transaction
>> and at the toplevel. I share your concerns about allowing config =
reload in
>> arbitrary places. While we might want to go there, it would require a =
lot more
>> analysis.

Thinking more on this I'm leaning towards going with allowing more =
frequent
reloads in autovacuum, and saving the same for VACUUM for more careful =
study.
The general case is probably fine but I'm not convinced that there =
aren't error
cases which can present unpleasant scenarios.

Regarding the autovacuum part of this patch I think we are down to the =
final
details and I think it's doable to finish this in time for 16.

> As an alternative for your consideration, attached v14 set implements
> the config file reload for autovacuum only (in 0003) and then enables =
it
> for VACUUM and ANALYZE not in a nested transaction command (in 0004).
>=20
> Previously I had the commits in the reverse order for ease of review =
(to
> separate changes to worker limit balancing logic from config reload
> code).

A few comments on top of already submitted reviews, will do another pass =
over
this later today.

+ * VacuumFailsafeActive is a defined as a global so that we can =
determine
+ * whether or not to re-enable cost-based vacuum delay when vacuuming a =
table.

This comment should be expanded to document who we expect to inspect =
this
variable in order to decide on cost-based vacuum.

Moving the failsafe switch into a global context means we face the risk =
of an
extension changing it independently of the GUCs that control it (or the =
code
relying on it) such that these are out of sync.  External code messing =
up
internal state is not new and of course outside of our control, but it's =
worth
at least considering.  There isn't too much we can do here, but perhaps =
expand
this comment to include a "do not change this" note?

+extern bool VacuumFailsafeActive;

While I agree with upthread review comments that extensions shoulnd't =
poke at
this, not decorating it with PGDLLEXPORT adds little protection and only =
cause
inconsistencies in symbol exports across platforms.  We only explicitly =
hide
symbols in shared libraries IIRC.

+extern int VacuumCostLimit;
+extern double VacuumCostDelay;
 ...
-extern PGDLLIMPORT int VacuumCostLimit;
-extern PGDLLIMPORT double VacuumCostDelay;

Same with these, I don't think this is according to our default =
visibility.
Moreover, I'm not sure it's a good idea to perform this rename.  This =
will keep
VacuumCostLimit and VacuumCostDelay exported, but change their meaning.  =
Any
external code referring to these thinking they are backing the GUCs will =
still
compile, but may be broken in subtle ways.  Is there a reason for not =
keeping
the current GUC variables and instead add net new ones?


+ * TODO: should VacuumCostLimit and VacuumCostDelay be initialized to =
valid or
+ * invalid values?
+ */
+int                    VacuumCostLimit =3D 0;
+double         VacuumCostDelay =3D -1;

I think the important part is to make sure they are never accessed =
without
VacuumUpdateCosts having been called first.  I think that's the case =
here, but
it's not entirely clear.  Do you see a codepath where that could happen? =
 If
they are initialized to a sentinel value we also need to check for that, =
so
initializing to the defaults from the corresponding GUCs seems better.

+* Update VacuumCostLimit with the correct value for an autovacuum =
worker, given

Trivial whitespace error in function comment.


+static double av_relopt_cost_delay =3D -1;
+static int av_relopt_cost_limit =3D 0;

These need a comment IMO, ideally one that explain why they are =
initialized to
those values.


+       /* There is at least 1 autovac worker (this worker). */
+       Assert(nworkers_for_balance > 0);

Is there a scenario where this is expected to fail?  If so I think this =
should
be handled and not just an Assert.

--
Daniel Gustafsson