On Thu, Jan 27, 2022 at 8:28 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> I'm sure you meant "&" here (fixed in attached patch to appease the cfbot):
> + if (options | VACOPT_MINIMAL)
Thanks for catching that! That copy-pasto was also masking my failure
to process the option properly -- fixed in the attached as v5.
> It should either refuse to run if a list of tables is specified with MINIMAL,
> or it should filter that list by XID condition.
I went with the former for simplicity. As a single-purpose option, it
makes sense.
> As for the name, it could be MINIMAL or FAILSAFE or EMERGENCY or ??
> I think the name should actually be a bit more descriptive, and maybe say XID,
> like MINIMAL_XID or XID_EMERGENCY...
I went with EMERGENCY in this version to reinforce its purpose in the
mind of the user (and reader of this code).
> Normally, options are independent, but VACUUM (MINIMAL) is a "shortcut" to a
> hardcoded set of options: freeze on, truncate off, cleanup off. So it refuses
> to be combined with other options - good.
>
> This is effectively a shortcut to hypothetical parameters for selecting tables
> by XID/MXID age. In the future, someone could debate adding user-facing knobs
> for table selection by age.
I used the params struct in v5 for the emergency cutoff ages. Even
with the values hard-coded, it seems cleaner to keep them here.
> I still wonder if the relations should be processed in order of decreasing age.
> An admin might have increased autovacuum_freeze_max_age up to 2e9, and your
> query might return thousands of tables, with a wide range of sizes and ages.
>
> Processing them in order of decreasing age would allow the admin to quickly
> vacuum the oldest tables, and optionally interrupt vacuum to get out of single
> user mode ASAP - even if their just want to run VACUUM(MINIMAL) in a normal
> backend when services aren't offline. Processing them out of order might be
> pretty surprising - they might run vacuum for an hour (or overnight), cancel
> it, attempt to start the DB in normal mode, and conclude that it made no
> visible progress.
While that seems like a nice property to have, it does complicate
things, so can be left for follow-on work.
Also in v5:
- It mentions the new command in the error hint in
GetNewTransactionId(). I'm not sure if multi-word commands should be
quoted like this.
- A first draft of documentation
--
John Naylor
EDB: http://www.enterprisedb.com