Re: do only critical work during single-user vacuum?

On Tue, Feb 01, 2022 at 04:50:31PM -0500, John Naylor wrote:
> On Thu, Jan 27, 2022 at 8:28 PM Justin Pryzby <pryzby(at)telsasoft(dot)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.

Thank the cfbot ;)

Actually, your most recent patch failed again without this:

- if (params->VACOPT_EMERGENCY)
+ if (params->options & VACOPT_EMERGENCY)

> - It mentions the new command in the error hint in
> GetNewTransactionId(). I'm not sure if multi-word commands should be
> quoted like this.

Use <literal> ?

> + xid age or mxid age is older than 1 billion. To complete as quickly as possible, an emergency
> + vacuum will skip truncation and index cleanup, and will skip toast tables whose age has not
> + exceeded the cutoff.

Why does this specially mention toast tables ?

> + While this option could be used while the postmaster is running, it is expected that the wraparound
> + failsafe mechanism will automatically work in the same way to prevent imminent shutdown.
> + When <literal>EMERGENCY</literal> is specified no tables may be listed, since it is designed to

specified comma

> + select candidate relations from the entire database.
> + The only other option that may be combined with <literal>VERBOSE</literal>, although in single-user mode no client messages are

this is missing a word?
Maybe say: May not be combined with any other option, other than VERBOSE.

Should the docs describe that the vacuum is done with cost based delay disabled
and with vacuum_freeze_table_age=0 (and other settings).

> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("option \"%s\" is incompatible with EMERGENCY", opt->defname),
> + parser_errposition(pstate, opt->location)));

IMO new code should avoid using the outer parenthesis around errcode().

Maybe the errmsg should say: .. may not be specified with EMERGENCY.
EMERGENCY probably shouldn't be part of the translatable string.

+ if (strcmp(opt->defname, "emergency") != 0 &&
+ strcmp(opt->defname, "verbose") != 0 &&
+ defGetBoolean(opt))

It's wrong to call getBoolean(), since the options may not be boolean.
postgres=# VACUUM(EMERGENCY, INDEX_CLEANUP auto);
ERROR: index_cleanup requires a Boolean value

I think EMERGENCY mode should disable process_toast. It already processes
toast tables separately. See 003.

Should probably exercise (EMERGENCY) in vacuum.sql. See 003.

> > 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.
>
> While that seems like a nice property to have, it does complicate
> things, so can be left for follow-on work.

I added that in the attached 003.

--
Justin