On Thu, Dec 20, 2018 at 04:48:11PM +0000, Bossart, Nathan wrote:
> The --skip-locked option in vacuumdb is part of 0002, so I don't think
> there's much precedent here.
It looks like I was not looking at the master branch 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?
I am not exactly sure down to which version this needs to be supported
and what's the policy about that (if others have an opinion to share,
please feel free) but surely it does not hurt to keep those code paths
either from a maintenance point of view.
> 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.
There are downsides and upsides for each approach. Reporting a failure
makes it clear that an option is not available with a clear error
message, however it complicates a bit the error handling for parallel
runs. And vacuum_one_database should be the code path doing as that's
where all the connections are taken even when all databases are
processed.
Ignoring the option, as your patch set does, has the disadvantage to
potentially confuse users, say we may see complains like "why is VACUUM
locking even if I specified --skip-locked?". Still this keeps the code
minimalistic and simple.
Just passing down the options and seeing a failure in the query
generated is also a confusing experience I guess for not-so-advanced
users even if it may simplify the error handling.
Issuing a proper error feels more natural to me I think, as users can
react on that properly. Opinions from others are welcome.
--
Michael