Hi,
Thank you for updating the patches.
On Thu, Mar 30, 2023 at 5:01 AM Melanie Plageman
<melanieplage...@gmail.com> wrote:
>
> Thanks for the detailed review!
>
> On Tue, Mar 28, 2023 at 11:09 PM Kyotaro Horiguchi
> <horikyota....@gmail.com> wrote:
> >
> > At Tue, 28 Mar 2023 20:35:28 -0400, Melanie Plageman
> > <melanieplage...@gmail.com> wrote in
> > > On Tue, Mar 28, 2023 at 4:21 AM Kyotaro Horiguchi
> > > <horikyota....@gmail.com> wrote:
> > > >
> > > > At Mon, 27 Mar 2023 14:12:03 -0400, Melanie Plageman
> > > > <melanieplage...@gmail.com> wrote in
> > > >
> > > > 0002:
> > > >
> > > > I felt a bit uneasy on this. It seems somewhat complex (and makes the
> > > > succeeding patches complex),
> > >
> > > Even if we introduced a second global variable to indicate that failsafe
> > > 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 != 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 = cost active
> failsafe inactive + cost inactive = cost inactive
> failsafe active + cost inactive = cost inactive and locked
> the fourth is invalid
> failsafe active + cost active = 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 = 0,
VACUUM_COST_INACTIVE,
VACUUM_COST_ACTIVE,
} VacuumCostStatus;
VacuumCostStatus VacuumCost;
non-vacuum code can use the following macros:
#define VacuumCostActive() (VacuumCost == VACUUM_COST_ACTIVE)
#define VacuumCostInactive() (VacuumCost <= 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,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
