Date: Wed, 22 Aug 2018 09:43:35 +0900
From: Michael Paquier <michael@paquier.xyz>
To: "Bossart, Nathan" <bossartn@amazon.com>
Cc: Postgres hackers <pgsql-hackers@postgresql.org>,
	"horiguchi.kyotaro@lab.ntt.co.jp" <horiguchi.kyotaro@lab.ntt.co.jp>
Subject: Re: Improve behavior of concurrent ANALYZE/VACUUM
Message-ID: <20180822004335.GA4333@paquier.xyz>
References: <20180812222142.GA6097@paquier.xyz>
 <20180817064223.GH1693@paquier.xyz>
 <720DF5E4-053F-46C7-B11F-550EF2C39F6D@amazon.com>
 <20180821012815.GB2897@paquier.xyz>
 <CAA861D8-68C1-4A2D-84E9-E2EE6B7B6A72@amazon.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="LQksG6bCIzRHxTLp"
Content-Disposition: inline
In-Reply-To: <CAA861D8-68C1-4A2D-84E9-E2EE6B7B6A72@amazon.com>
Precedence: bulk


--LQksG6bCIzRHxTLp
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Aug 21, 2018 at 04:01:50PM +0000, Bossart, Nathan wrote:
> I think my biggest concern with this approach is that we'd be
> introducing inconsistent behavior whenever there are concurrent
> changes.  If a user never had permissions to VACUUM the partitioned
> table, the partitions are skipped outright.  However, if the user
> loses permissions to VACUUM the partitioned table between
> expand_vacuum_rel() and vacuum_rel(), we'll still attempt to VACUUM
> each individual partition.
>=20
> I'll admit I don't have a great alternative proposal that doesn't
> involve adding deadlock risk or complexity, but it still seems worth
> mulling over.

That counts only for a manual vacuum/analyze listing directly the
relation in question.  If running a system-wide VACUUM then all the
relations are still processed.  This is a rather edge case in my opinion
but..  I don't mind mulling over it (as you say).  So please let me
think over it for a couple of days.  I don't see a smart solution which
does not create risks of lock upgrades and deadlocks now, there may be
one able to preserve the existing behavior.

>> I have split the patch into two parts:
>> - 0001 includes new tests which generate WARNING messages for VACUUM,
>> ANALYZE and VACUUM (ANALYZE).  That's useful separately.
>=20
> 0001 looks good to me.

Thanks, I have pushed this one.

>> - 0002 is the original patch discussed here.
>=20
> I'd suggest even splitting 0002 into two patches: one for refactoring
> the existing permissions checks into vacuum_is_relation_owner() and
> another for the new checks.

Hmmm.  The second patch changes also some comment blocks when calling
vacuum_is_relation_owner(), so we finish by changing the same code
areas, resulting in more code churn for no real gain.

> +# The role doesn't have privileges to vacuum the table, so VACUUM should
> +# immediately skip the table without waiting for a lock.
>=20
> Can we add tests for concurrent changes that cause the relation to be
> skipped in vacuum_rel() and analyze_rel() instead of
> expand_vacuum_rel()?

Doing that deterministically with concurrent tests look difficult to me
as doing ALTER TABLE OWNER TO to a relation in a first session causes a
second session running VACUUM to block in expand_vacuum_rel(), be it
with a plain table or a partitioned table (doing the ALTER TABLE on a
leaf will block scanning the parent as well).
--
Michael

--LQksG6bCIzRHxTLp
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlt8sbcACgkQnvQgOdby
QH3KHw//TWuC0lfBfBbfEmNvVkG07aunsaXjxcmeFrNTFzkNUAPiycb2fEg853Ri
lDfixWix+cSAOG7zQKtC7+/Yx/bFilY3NVmZ0YG7BDQWzSWCdqnpBpP1wNBcknuw
k7zq6vBH01PuKRwBvK5R9h+LJkWYbpOUI/udMHMSGudJQcLhtcP8OkVPY2fHRiBV
rPuv4riUt3ntkucVjAuDxXowp92GXvV2YOZnSoEYuwn0rPKiM7ncYCTKsGx3UI0l
MrQUrJDmnAijBKP/34o1D8GdVRkWVFF+DAkWuoIIZLYkwBFCox8A+Wk1VxtRLaVZ
OPPW7TWupCFJxdp2J74GzLM8E/WRHNqM2dOwbPqzWsttemCtbfWJ9EFcEpLHfDVH
BJTsdrKHCHnAo+FMSXKvXeTXl1N5yljXXjVekNqTrbcZRENYyT9SpOaOuZR+/yB9
YJhymETYDwLiZAIBGMkE9AKKAL/xCpkK8WTtMknSIjnkTXDvRylU6zQMLlWG/QjS
7Wn9Ii+ljV/eUNWY0dIO3esPmq58PJL7Tq0onJyxIFRKBA+Wltj/SWTiiI99XA7j
uGCriQ+BqyFPD3SPjkHo4+NakG7f3TPM+IgfRHrBevdIWmgmiQ/dNObhF41MMKb9
XSP9q8RTfjA51BJo5ZF6eMoEU1Q1N0S2F7RkwplNRrEwuk6pKNg=
=3hVQ
-----END PGP SIGNATURE-----

--LQksG6bCIzRHxTLp--