Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 14 Jun 2023 08:16:15 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Jeff Davis <pgsql@j-davis.com>, Ted Yu <yuzhihong@gmail.com>,
	Pavel Luzanov <p.luzanov@postgrespro.ru>,
	Justin Pryzby <pryzby@telsasoft.com>, pgsql-hackers@postgresql.org
Subject: Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
Message-ID: <ZIj4v1CwqlDVJZfB@paquier.xyz>
References: <CALte62zW+s+V3fDUxP2ZYk2f=DZsyZO-Aa_bbiA39=wrz=UHuQ@mail.gmail.com>
 <20221218233018.GA1476904@nathanxps13>
 <20230103234549.GA289060@nathanxps13>
 <20230109225157.GA1288965@nathanxps13>
 <7d2a8b72e23c8236281c6e00ae790327f965a8e5.camel@j-davis.com>
 <20230113203334.GA2206335@nathanxps13>
 <e25c84fafae4eb08d8a6e83207678096bac102d6.camel@j-davis.com>
 <20230113225626.GA2380176@nathanxps13>
 <20230113231339.GA2422750@nathanxps13>
 <20230613211246.GA219055@nathanxps13>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="w+YkIt2VkfLrTO6Y"
Content-Disposition: inline
In-Reply-To: <20230613211246.GA219055@nathanxps13>
Archived-At: <https://www.postgresql.org/message-id/ZIj4v1CwqlDVJZfB%40paquier.xyz>
Precedence: bulk


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

On Tue, Jun 13, 2023 at 02:12:46PM -0700, Nathan Bossart wrote:
> I've been reviewing ff9618e lately, and I'm wondering whether it has the
> same problem that 19de0ab solved.  Specifically, ff9618e introduces
> has_partition_ancestor_privs(), which is used to check whether a user has
> MAINTAIN on any partition ancestors.  This involves syscache lookups, and
> presently this function does not take any relation locks.  I did spend so=
me
> time trying to induce cache lookup errors, but I didn't have any luck.
> However, unless this can be made safe without too much trouble, I think I=
'm
> inclined to partially revert ff9618e, leaving the TOAST-related parts
> intact.

Hmm.  get_rel_relispartition() and pg_class_aclcheck() are rather
reliable when it comes to that as far as it goes.  Still
get_partition_ancestors() is your problem, isn't it?  Indeed, it seems
like a bad idea to do partition tree lookups without at least an
AccessShareLock as you may finish with a list that makes
pg_class_aclcheck() complain on a missing relation.  The race is
pretty narrow, but a stop point in get_partition_ancestors() with some
partition tree manipulation should be enough to make operations like a
schema-wide REINDEX less transparent with missing relations at least.

has_partition_ancestor_privs() is used in
RangeVarCallbackMaintainsTable(), on top of that.  As written, it
encourages incorrect use patterns.

> By reverting the partition-related parts of ff9618e, users would need to
> have MAINTAIN on the partition itself to perform the maintenance command.
> MAINTAIN on the partitioned table would no longer be sufficient.  This is
> more like how things work on supported versions today.  Privileges are
> checked for each partition, so a command that flows down to all partitions
> might refuse to process a partition (e.g., if the current user doesn't own
> the partition).
>=20
> In the future, perhaps we could reevaluate adding these partition ancestor
> privilege checks, but I'd rather leave it out for now instead of
> introducing behavior in v16 that is potentially buggy and difficult to
> remove post-GA.

While on it, this buzzes me:
 static bool
-vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
+vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params, bool skip_=
privs)

VacuumParams has been originally introduced to avoid extending
vacuum_rel() with a bunch of arguments, no?

So, yes, agreed about the removal of has_partition_ancestor_privs().
I am adding an open item assigned to you and Jeff.
--
Michael

--w+YkIt2VkfLrTO6Y
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmSI+L8ACgkQnvQgOdby
QH37HRAAp+q7wOakduHIbHBxNx8ZctMwPKZ9dEAInIp4F91Bcks0GBxhLYKsuTHU
8fn3ZKVoiAFTHw1fWJGcVOcxq4bC+euywv9hwOvzm70Fkd5A2WSzHrBnT0joC71W
q09JNvQjiUOZzshtKKqpXAbPw2VZPND2dz05DpmBfto6/eQ+t4+2T8Y+qwHVYytW
d1+A0uP089ht6fEZeq4qUYqoy6Y4rNSG0BYiVdAx2tr8p+8TIU7/k6hIHETKmWIJ
HKVDyl1w0Ixyn1xXetd+qzAVF7MNugAf/wmIcy8v0qHhFPPxnCFl2mm/AS6a5oW1
JiTa1Chy82p4pVLZ/jgbq1U1RoN5Yh9qUAPl+SFqzlkwswORObE5YXsh0C2vVMLo
CESggM12O75hs3leBp9E9qNman+vlhPXn2b8Nmr8mnzpMgnajIkjZGmKJ0GVAfW/
HdI/rx/X+kZI4SmZBuXDmsfQ4/5KUJl8wf4bZ3zvluw9WZAbsMNEifeIaAqToQWV
CA1KFQGnBvkfFfoKSmv56xqNbIWigFWhkWbOjpmerWXxAZtQq4shYNoPlIUEOP7M
cEeu3I9AujEVJtRcGofpO9p5YS6h5CFd87Ji0gWfbEn1NecO3TXknY+ndWo19QkV
cHaBqOjz1sn0e2BZlpRii8bCSx7AxmZqmy3hgg/5K6MGnFGDM6I=
=Me4l
-----END PGP SIGNATURE-----

--w+YkIt2VkfLrTO6Y--