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>
In-Reply-To: <CAAKRu_Yg4-kWH_EN7wtpm3z9B+H27UTmOxzzYv1Vg=YdZ6v_rw@mail.gmail.com>
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Thu, 30 Mar 2023 11:57:05 +0900
Message-ID: <CAD21AoCGdxYAJHmE_nXw_MFw1+Qtn3vcThRu5FiadukgQnpWeQ@mail.gmail.com>
Subject: Re: Should vacuum process config file reload more often
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, daniel@yesql.se, pgsql-hackers@postgresql.org, 
	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/CAD21AoCGdxYAJHmE_nXw_MFw1%2BQtn3vcThRu5FiadukgQnpWeQ%40mail.gmail.com>
Precedence: bulk

Hi,

Thank you for updating the patches.

On Thu, Mar 30, 2023 at 5:01=E2=80=AFAM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> Thanks for the detailed review!
>
> On Tue, Mar 28, 2023 at 11:09=E2=80=AFPM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> >
> > At Tue, 28 Mar 2023 20:35:28 -0400, Melanie Plageman <melanieplageman@g=
mail.com> wrote in
> > > On Tue, Mar 28, 2023 at 4:21=E2=80=AFAM Kyotaro Horiguchi <horikyota.=
ntt@gmail.com> wrote:
> > > >
> > > > At Mon, 27 Mar 2023 14:12:03 -0400, Melanie Plageman <melanieplagem=
an@gmail.com> wrote in
> > > >
> > > > 0002:
> > > >
> > > > I felt a bit uneasy on this. It seems somewhat complex (and makes t=
he
> > > > succeeding patches complex),
> > >
> > > Even if we introduced a second global variable to indicate that fails=
afe
> > > mode has been engaged, we would still require the additional checks
> > > of VacuumCostInactive.
> > >
> > > > has confusing names,
> > >
> > > I would be happy to rename the values of the enum to make them less
> > > confusing. Are you thinking "force" instead of "locked"?
> > > maybe:
> > > VACUUM_COST_FORCE_INACTIVE and
> > > VACUUM_COST_INACTIVE
> > > ?
> > >
> > > > and doesn't seem like self-contained.
> > >
> > > By changing the variable from VacuumCostActive to VacuumCostInactive,=
 I
> > > have kept all non-vacuum code from having to distinguish between it
> > > being inactive due to failsafe mode or due to user settings.
> >
> > My concern is that VacuumCostActive is logic-inverted and turned into
> > a ternary variable in a subtle way. The expression
> > "!VacuumCostInactive" is quite confusing. (I sometimes feel the same
> > way about "!XLogRecPtrIsInvalid(lsn)", and I believe most people write
> > it with another macro like "lsn !=3D InvalidXLogrecPtr"). Additionally,
> > the constraint in this patch will be implemented as open code.  So I
> > wanted to suggest something like the attached. The main idea is to use
> > a wrapper function to enforce the restriction, and by doing so, we
> > eliminated the need to make the variable into a ternary without a good
> > reason.
>
> So, the rationale for making it a ternary is that the variable is the
> combination of two pieces of information which has only has 3 valid
> states:
> failsafe inactive + cost active =3D cost active
> failsafe inactive + cost inactive =3D cost inactive
> failsafe active + cost inactive =3D cost inactive and locked
> the fourth is invalid
> failsafe active + cost active =3D invalid
> That is harder to enforce with two variables.
> Also, the two pieces of information are not meaningful individually.
> So, I thought it made sense to make a single variable.
>
> Your suggested patch introduces an additional variable which shadows
> LVRelState->failsafe_active but doesn't actually get set/reset at all of
> the correct places. If we did introduce a second global variable, I
> don't think we should also keep LVRelState->failsafe_active, as keeping
> them in sync will be difficult.
>
> As for the double negative (!VacuumCostInactive), I agree that it is not
> ideal, however, if we use a ternary and keep VacuumCostActive, there is
> no way for non-vacuum code to treat it as a boolean.
> With the ternary VacuumCostInactive, only vacuum code has to know about
> the distinction between inactive+failsafe active and inactive+failsafe
> inactive.

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.

Or is there any reason why we need to keep VacuumCostActive and treat
it as a boolean?

Regards,

--=20
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com