On Thu, Mar 30, 2023 at 3:26 PM Daniel Gustafsson <dan...@yesql.se> wrote:
>
> > On 30 Mar 2023, at 04:57, Masahiko Sawada <sawada.m...@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 = 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.
>
> 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-value is
> easily misunderstood. A VacuumCostState tri-value variable (or a better name)
> with a set of convenient macros for extracting the boolean active/inactive
> 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
