Date: Fri, 21 Dec 2018 10:05:45 +0900
From: Michael Paquier <michael@paquier.xyz>
To: "Bossart, Nathan" <bossartn@amazon.com>
Cc: "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: A few new options for vacuumdb
Message-ID: <20181221010545.GD1886@paquier.xyz>
References: <FFE5373C-E26A-495B-B5C8-911EC4A41C5E@amazon.com>
 <20181220020522.GM27104@paquier.xyz>
 <7B4ACE8E-5D81-416B-B984-17E60F7ABABA@amazon.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="AbQceqfdZEv+FvjW"
Content-Disposition: inline
In-Reply-To: <7B4ACE8E-5D81-416B-B984-17E60F7ABABA@amazon.com>
Precedence: bulk


--AbQceqfdZEv+FvjW
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--AbQceqfdZEv+FvjW
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlwcPGkACgkQnvQgOdby
QH07yg/8C3uQJe5/cjIW3durFrzWdcgLVJyFrVbf14c6Oc596Hk1m6YU9ZDJmqGd
dvseCNGPTIFY/HQSoFgkB8MHdZWrAOL7G+iAT9exhVHjl8+OmR/yTOHktZtJ/3Y6
MGITyKqLwexdjufAJVRYY2ZJ4khRt3GyjqHSfc5GyjQQJb5GcQjlutvBYigKRxrQ
60CN0m9pMcJIYwQoMMJv0TVeyIUlzyOeUZzl2RAaFDkLXGHq0pxMHpEjzian3CGf
Ukna1Y2BePOpDnPfPkFXyybV9GVQW/DAdc9LjDPBUg5nA7qjMhZRvGbb9TBm0r44
66zidW5CeXzC/S+TUney6LHyT7CLfE2bZSJoag5O8pwj3YlGifWmVwgJjyv0XKBs
Bx08YG9XnU0OLr2w3DyHNSidJpGBhd2gMZjLC6GGle5eNOdagAixpnvzawTGwpW0
4ee5OsAbyV3AP/9g2Gf2w9PSaZcxTwBsZnBNl9WuLv0KhKNEMEl0J4+Yl+3zs3Jv
jzDwKbFPJ3akb9XDoWgbY7amAi+mrwDXhzNsatayIuAZeflsTP4wpMhny4c/RrVo
qQS1/QJVIn+OAd/rqxMip4HPg4MZpBMzJ8NqaLcalRJ4FJyHhzoEBkBZKzUJRtkK
lYqGNirPuEovQLtsTKx2eGLU5qDxPlOrmd67VIr5Fjc9y1TNZQQ=
=q4FX
-----END PGP SIGNATURE-----

--AbQceqfdZEv+FvjW--