public inbox for [email protected]  

help / color / mirror / Atom feed

From: Melanie Plageman <[email protected]>
To: Daniel Gustafsson <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Cc: Kyotaro Horiguchi <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: [email protected]
Subject: Re: Should vacuum process config file reload more often
Date: Fri, 31 Mar 2023 10:31:10 -0400
Message-ID: <CAAKRu_aoa0YT36q2n3xa9MZYdepeH4Jyf2EVJj=7YdC_1c974Q@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAAKRu_ZnG2nYcvzb4a-CGwg33_4RX83qitGGVmjhHZZtqeSrRA@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<CAAKRu_Yg4-kWH_EN7wtpm3z9B+H27UTmOxzzYv1Vg=YdZ6v_rw@mail.gmail.com>
	<CAD21AoCGdxYAJHmE_nXw_MFw1+Qtn3vcThRu5FiadukgQnpWeQ@mail.gmail.com>
	<[email protected]>

On Thu, Mar 30, 2023 at 3:26 PM Daniel Gustafsson <[email protected]> wrote:
>
> > On 30 Mar 2023, at 04:57, Masahiko Sawada <[email protected]> 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

view thread (27+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Should vacuum process config file reload more often
  In-Reply-To: <CAAKRu_aoa0YT36q2n3xa9MZYdepeH4Jyf2EVJj=7YdC_1c974Q@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox