Re: A few new options for vacuumdb

Hi Michael,

Thanks for taking a look.

On 12/19/18, 8:05 PM, "Michael Paquier" <michael(at)paquier(dot)xyz> wrote:
> On Wed, Dec 19, 2018 at 08:50:10PM +0000, Bossart, Nathan wrote:
>> If an option is specified for a server version that is not supported,
>> the option is silently ignored. For example, SKIP_LOCKED was only
>> added to VACUUM and ANALYZE for v12. Alternatively, I think we could
>> fail in vacuum_one_database() if an unsupported option is specified.
>> Some of these options will work on all currently supported versions,
>> so I am curious what others think about skipping some of these version
>> checks altogether.
>
> prepare_vacuum_command() already handles that by ignoring silently
> unsupported options (see the case of SKIP_LOCKED). So why not doing the
> same?

The --skip-locked option in vacuumdb is part of 0002, so I don't think
there's much precedent here. We do currently fall back to the
unparenthesized syntax for VACUUM for versions before 9.0, so there is
some version handling already. What do you think about removing
version checks for unsupported major versions? If we went that route,
these new patches could add version checks only for options that were
added in a supported major version (e.g. mxid_age() was added in 9.5).
Either way, we'll still have to decide whether to fail or to silently
skip the option if you do something like specify --min-mxid-age for a
9.4 server.

>> It does not seem clear whether the user wants us to process mytable
>> only if it is at least 1 GB, or if we should process mytable in
>> addition to any other relations over 1 GB. Either way, I think trying
>> to support these combinations of options adds more complexity than it
>> is worth.
>
> It seems to me that a combination of both options means that the listed
> table should be processed only if its minimum size is 1GB. If multiple
> tables are specified with --table, then only those reaching 1GB would be
> processed. So this restriction can go away. The same applies for the
> proposed --min-xid-age and --min-mxid-age.

Okay. This should be pretty easy to do using individual catalog
lookups before we call prepare_vacuum_command(). Alternatively, I
could add a clause for each specified table in the existing query in
vacuum_one_database(). At that point, it may be cleaner to just use
that catalog query in all cases instead of leaving paths where tables
can remain NULL.

> + <para>
> + Only execute the vacuum or analyze commands on tables with a multixact
> + ID age of at least <replaceable
> class="parameter">mxid_age</replaceable>.
> + </para>
> Adding a link to explain the multixact business may be helpful, say
> vacuum-for-multixact-wraparound. Same comment for XID.

Good idea.

> There is an argument about also adding DISABLE_PAGE_SKIPPING.

I'll add this in the next set of patches. I need to add the
parenthesized syntax and --skip-locked for ANALYZE in
prepare_vacuum_command(), too.

Nathan