On Mon, Aug 29, 2022 at 3:40 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> Agreed.
Attached is v2, which cleans up the structure of
vacuum_set_xid_limits() a bit more. The overall idea was to improve
the overall flow/readability of the function by moving the WARNINGs
into their own "code stanza", just after the logic for establishing
freeze cutoffs and just before the logic for deciding on
aggressiveness. That is now the more logical approach (group the
stanzas by functionality), since we can't sensibly group the code
based on whether it deals with XIDs or with Multis anymore (not since
the function was taught to deal with the question of whether caller's
VACUUM will be aggressive).
Going to push this in the next day or so.
I also removed some local variables that seem to make the function a
lot harder to follow in v2. Consider code like this:
- freezemin = freeze_min_age;
- if (freezemin < 0)
- freezemin = vacuum_freeze_min_age;
- freezemin = Min(freezemin, autovacuum_freeze_max_age / 2);
- Assert(freezemin >= 0);
+ if (freeze_min_age < 0)
+ freeze_min_age = vacuum_freeze_min_age;
+ freeze_min_age = Min(freeze_min_age, autovacuum_freeze_max_age / 2);
+ Assert(freeze_min_age >= 0);
Why have this freezemin temp variable? Why not just use the
vacuum_freeze_min_age function parameter directly instead? That is a
better representation of what's going on at the conceptual level. We
now assign vacuum_freeze_min_age to the vacuum_freeze_min_age arg (not
to the freezemin variable) when our VACUUM caller passes us a value of
-1 for that arg. -1 effectively means "whatever the value of the
vacuum_freeze_min_age GUC is'', which is clearer without the
superfluous freezemin variable.
--
Peter Geoghegan
