MIME-Version: 1.0
References: <CAAKRu_ZnG2nYcvzb4a-CGwg33_4RX83qitGGVmjhHZZtqeSrRA@mail.gmail.com>
 <20230329.120908.350115307125624430.horikyota.ntt@gmail.com>
 <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>
In-Reply-To: <935191FB-083B-4060-89FC-466F61FAA391@yesql.se>
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Fri, 31 Mar 2023 10:31:10 -0400
Message-ID: <CAAKRu_aoa0YT36q2n3xa9MZYdepeH4Jyf2EVJj=7YdC_1c974Q@mail.gmail.com>
Subject: Re: Should vacuum process config file reload more often
To: Daniel Gustafsson <daniel@yesql.se>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, 
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>, Andres Freund <andres@anarazel.de>, 
	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_aoa0YT36q2n3xa9MZYdepeH4Jyf2EVJj%3D7YdC_1c974Q%40mail.gmail.com>
Precedence: bulk

On Thu, Mar 30, 2023 at 3:26=E2=80=AFPM Daniel Gustafsson <daniel@yesql.se>=
 wrote:
>
> > On 30 Mar 2023, at 04:57, Masahiko Sawada <sawada.mshk@gmail.com> wrote=
:
>
> > As another idea, why don't we use macros for that? For example,
> > suppose VacuumCostStatus is like:
> >
> > typedef enum VacuumCostStatus
> > {
> >    VACUUM_COST_INACTIVE_LOCKED =3D 0,
> >    VACUUM_COST_INACTIVE,
> >    VACUUM_COST_ACTIVE,
> > } VacuumCostStatus;
> > VacuumCostStatus VacuumCost;
> >
> > non-vacuum code can use the following macros:
> >
> > #define VacuumCostActive() (VacuumCost =3D=3D VACUUM_COST_ACTIVE)
> > #define VacuumCostInactive() (VacuumCost <=3D VACUUM_COST_INACTIVE) //
> > or we can use !VacuumCostActive() instead.
>
> I'm in favor of something along these lines.  A variable with a name that
> implies a boolean value (active/inactive) but actually contains a tri-val=
ue is
> easily misunderstood.  A VacuumCostState tri-value variable (or a better =
name)
> with a set of convenient macros for extracting the boolean active/inactiv=
e that
> most of the code needs to be concerned with would more for more readable =
code I
> think.

The macros are very error-prone. I was just implementing this idea and
mistakenly tried to set the macro instead of the variable in multiple
places. Avoiding this involves another set of macros, and, in the end, I
think the complexity is much worse. Given the reviewers' uniform dislike
of VacuumCostInactive, I favor going back to two variables
(VacuumCostActive + VacuumFailsafeActive) and moving
LVRelState->failsafe_active to the global VacuumFailsafeActive.

I will reimplement this in the next version.

On the subject of globals, the next version will implement
Horiguchi-san's proposal to separate GUC variables from the globals used
in the code (quoted below). It should hopefully reduce the complexity of
this patchset.

> Although it's somewhat unrelated to the goal of this patch, I think we
> should clean up the code tidy before proceeding. Shouldn't we separate
> the actual parameters from the GUC base variables, and sort out the
> all related variaghble? (something like the attached, on top of your
> patch.)

- Melanie