Thread

Commits

Improve privilege documentation for maintenance commands.
- c2122aae636d 16.0 landed
Fix cache lookup hazards introduced by ff9618e82a.
- 4dbdb82513b6 16.0 landed
Move bool parameter for vacuum_rel() to option bits.
- 5b1a87994312 16.0 landed
Fix MAINTAIN privileges for toast tables and partitions.
- ff9618e82a46 16.0 landed
Add grantable MAINTAIN privilege and pg_maintain role.
- 60684dd834a2 16.0 landed

allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2022-12-08T18:37:07Z

Hi hackers,

This is meant as a continuation of the work to make VACUUM and ANALYZE
grantable privileges [0].  As noted there, the primary motivation for this
is to continue chipping away at things that require special privileges or
even superuser.  I've attached two patches.  0001 makes it possible to
grant CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX.  0002 adds
predefined roles that allow performing these commands on all relations.
After applying these patches, there are 13 privilege bits remaining for
future use.

There is an ongoing discussion in another thread [1] about how these
privileges should be divvied up.  Should each command get it's own
privilege bit (as I've done in the attached patches), or should the
privileges be grouped in some fashion (e.g., adding a MAINTAIN bit that
governs all of them, splitting out exclusive-lock operations from
non-exclusive-lock ones)?

Most of the changes in the attached patches are rather mechanical, and like
VACUUM/ANALYZE, there is room for future enhancement, such as granting the
privileges on databases/schemas instead of just tables.

[0] https://postgr.es/m/20220722203735.GB3996698%40nathanxps13
[1] https://postgr.es/m/20221206193606.GB3078082%40nathanxps13

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Jeff Davis <pgsql@j-davis.com> — 2022-12-10T20:07:12Z

On Thu, 2022-12-08 at 10:37 -0800, Nathan Bossart wrote:
> 0001 makes it possible to
> grant CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX.  0002 adds
> predefined roles that allow performing these commands on all
> relations.

Regarding the pg_refresh_all_matview predefined role, I don't think
it's a good idea. Refreshing a materialized view doesn't seem like an
administrative action to me.

First, it's unbounded in time, so the admin would need to be careful to
have a timeout. Second, the freshness of a materialized view seems very
specific to the application, rather than something that an admin would
have a blanket policy about. Thirdly, there's not a lot of information
the admin could use to make decisions about when to refresh (as opposed
to VACUUM/CLUSTER/REINDEX, where the stats are helpful).

But I'm fine with having a grantable privilege to refresh a
materialized view.

It seems like the discussion on VACUUM/CLUSTER/REINDEX privileges is
happening in the other thread. What would you like to accomplish in
this thread?

-- 
Jeff Davis
PostgreSQL Contributor Team - AWS

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2022-12-10T20:41:09Z

On Sat, Dec 10, 2022 at 12:07:12PM -0800, Jeff Davis wrote:
> It seems like the discussion on VACUUM/CLUSTER/REINDEX privileges is
> happening in the other thread. What would you like to accomplish in
> this thread?

Given the feedback in the other thread [0], I was planning to rewrite this
patch to create a MAINTAIN privilege and a pg_maintain_all_tables
predefined role that allowed VACUUM, ANALYZE, CLUSTER, REFRESH MATERIALIZED
VIEW, and REINDEX.

[0] https://postgr.es/m/20221206193606.GB3078082%40nathanxps13

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2022-12-12T20:04:27Z

On Sat, Dec 10, 2022 at 12:41:09PM -0800, Nathan Bossart wrote:
> On Sat, Dec 10, 2022 at 12:07:12PM -0800, Jeff Davis wrote:
>> It seems like the discussion on VACUUM/CLUSTER/REINDEX privileges is
>> happening in the other thread. What would you like to accomplish in
>> this thread?
> 
> Given the feedback in the other thread [0], I was planning to rewrite this
> patch to create a MAINTAIN privilege and a pg_maintain_all_tables
> predefined role that allowed VACUUM, ANALYZE, CLUSTER, REFRESH MATERIALIZED
> VIEW, and REINDEX.

Patch attached.  I ended up reverting some parts of the VACUUM/ANALYZE
patch that were no longer needed (i.e., if the user doesn't have permission
to VACUUM, we don't need to separately check whether the user has
permission to ANALYZE).  Otherwise, I don't think there's anything
tremendously different between v1 and v2 besides the fact that all the
privileges are grouped together.

Since there are only 15 privilege bits used after this patch is applied,
presumably we could revert widening AclMode to 64 bits.  However, I imagine
that will still be necessary at some point in the near future, so I don't
see a strong reason to revert it.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2022-12-12T21:01:36Z

On Mon, Dec 12, 2022 at 12:04:27PM -0800, Nathan Bossart wrote:
> Patch attached.  I ended up reverting some parts of the VACUUM/ANALYZE
> patch that were no longer needed (i.e., if the user doesn't have permission
> to VACUUM, we don't need to separately check whether the user has
> permission to ANALYZE).  Otherwise, I don't think there's anything
> tremendously different between v1 and v2 besides the fact that all the
> privileges are grouped together.

Here is a v3 of the patch that fixes a typo in the docs.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Jeff Davis <pgsql@j-davis.com> — 2022-12-14T03:05:10Z

On Mon, 2022-12-12 at 13:01 -0800, Nathan Bossart wrote:
> On Mon, Dec 12, 2022 at 12:04:27PM -0800, Nathan Bossart wrote:
> > Patch attached.  I ended up reverting some parts of the
> > VACUUM/ANALYZE
> > patch that were no longer needed (i.e., if the user doesn't have
> > permission
> > to VACUUM, we don't need to separately check whether the user has
> > permission to ANALYZE).  Otherwise, I don't think there's anything
> > tremendously different between v1 and v2 besides the fact that all
> > the
> > privileges are grouped together.
> 
> Here is a v3 of the patch that fixes a typo in the docs.

Committed.

The only significant change is that it also allows LOCK TABLE if you
have the MAINTAIN privilege.

I noticed a couple issues unrelated to your patch, and started separate
patch threads[1][2].

[1] 
https://www.postgresql.org/message-id/c0a85c2e83158560314b576b6241c8ed0aea1745.camel@j-davis.com
[2]
https://www.postgresql.org/message-id/9550c76535404a83156252b25a11babb4792ea1e.camel@j-davis.com

-- 
Jeff Davis
PostgreSQL Contributor Team - AWS

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2022-12-14T03:23:32Z

On Tue, Dec 13, 2022 at 07:05:10PM -0800, Jeff Davis wrote:
> Committed.
> 
> The only significant change is that it also allows LOCK TABLE if you
> have the MAINTAIN privilege.

Thanks!

> I noticed a couple issues unrelated to your patch, and started separate
> patch threads[1][2].

Will take a look.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Pavel Luzanov <p.luzanov@postgrespro.ru> — 2022-12-14T09:07:13Z

After a fresh install, including the patch for \dpS [1],
I found that granting MAINTAIN privilege does not allow the TOAST table 
to be vacuumed.

postgres@postgres(16.0)=# GRANT MAINTAIN ON pg_type TO alice;
GRANT
postgres@postgres(16.0)=# \c - alice
You are now connected to database "postgres" as user "alice".
alice@postgres(16.0)=> \dpS pg_type
                                     Access privileges
    Schema   |  Name   | Type  |     Access privileges      | Column 
privileges | Policies
------------+---------+-------+----------------------------+-------------------+----------
  pg_catalog | pg_type | table | 
postgres=arwdDxtm/postgres+|                   |
             |         |       | =r/postgres +|                   |
             |         |       | alice=m/postgres |                   |
(1 row)

So, the patch for \dpS works as expected and can be committed.

alice@postgres(16.0)=> VACUUM pg_type;
WARNING:  permission denied to vacuum "pg_toast_1247", skipping it
VACUUM

[1] 
https://www.postgresql.org/message-id/20221206193606.GB3078082%40nathanxps13

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2022-12-14T17:13:36Z

On Wed, Dec 14, 2022 at 12:07:13PM +0300, Pavel Luzanov wrote:
> I found that granting MAINTAIN privilege does not allow the TOAST table to
> be vacuumed.

Hm.  My first thought is that this is the appropriate behavior.  WDYT?

> So, the patch for \dpS works as expected and can be committed.

Thanks for reviewing.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-12-14T18:05:34Z

On 2022-Dec-14, Nathan Bossart wrote:

> On Wed, Dec 14, 2022 at 12:07:13PM +0300, Pavel Luzanov wrote:
> > I found that granting MAINTAIN privilege does not allow the TOAST table to
> > be vacuumed.
> 
> Hm.  My first thought is that this is the appropriate behavior.  WDYT?

It seems wrong to me.  If you can vacuum a table, surely you can also
vacuum its toast table.  If you can vacuum all normal tables, you should
be able to vacuum all toast tables too.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"La virtud es el justo medio entre dos defectos" (Aristóteles)

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2022-12-14T18:16:59Z

On Wed, Dec 14, 2022 at 07:05:34PM +0100, Alvaro Herrera wrote:
> On 2022-Dec-14, Nathan Bossart wrote:
>> On Wed, Dec 14, 2022 at 12:07:13PM +0300, Pavel Luzanov wrote:
>> > I found that granting MAINTAIN privilege does not allow the TOAST table to
>> > be vacuumed.
>> 
>> Hm.  My first thought is that this is the appropriate behavior.  WDYT?
> 
> It seems wrong to me.  If you can vacuum a table, surely you can also
> vacuum its toast table.  If you can vacuum all normal tables, you should
> be able to vacuum all toast tables too.

Okay.  Should all the privileges governed by MAINTAIN apply to a relation's
TOAST table as well?  I would think so, but I don't want to assume too much
before writing the patch.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Jeff Davis <pgsql@j-davis.com> — 2022-12-14T19:05:13Z

On Wed, 2022-12-14 at 10:16 -0800, Nathan Bossart wrote:
> Okay.  Should all the privileges governed by MAINTAIN apply to a
> relation's
> TOAST table as well?

Yes, I agree.


-- 
Jeff Davis
PostgreSQL Contributor Team - AWS

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Jeff Davis <pgsql@j-davis.com> — 2022-12-14T19:46:54Z

On Wed, 2022-12-14 at 12:07 +0300, Pavel Luzanov wrote:
> After a fresh install, including the patch for \dpS [1],
> I found that granting MAINTAIN privilege does not allow the TOAST
> table 
> to be vacuumed.

I wanted to also mention partitioning. The behavior is that MAINTAIN
privileges on the partitioned table does not imply MAINTAIN privileges
on the partitions. I believe that's fine and it's consistent with other
privileges on partitioned tables, such as SELECT and INSERT. In the
case of an admin maintaining users' tables, they'd be a member of
pg_maintain anyway.

Furthermore, MAINTAIN privileges on the partitioned table do not grant
the ability to create new partitions. There's a comment in tablecmds.c
alluding to a possible "UNDER" privilege:

  /*                                                                  
   * We should have an UNDER permission flag for this, but for now,   
   * demand that creator of a child table own the parent.             
   */

Perhaps there's something we want to do there, but it's a different use
case than the MAINTAIN privilege, so I don't see a reason it should be
grouped. Also, there's a bit of weirdness to think about in cases where
another user creates (and owns) a partition of your table (currently
this is only possible if the other user is a superuser).

I am not suggesting a change here, just posting in case someone has a
different opinion.

-- 
Jeff Davis
PostgreSQL Contributor Team - AWS

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Isaac Morland <isaac.morland@gmail.com> — 2022-12-14T20:32:45Z

On Wed, 14 Dec 2022 at 14:47, Jeff Davis <pgsql@j-davis.com> wrote:

Furthermore, MAINTAIN privileges on the partitioned table do not grant
> the ability to create new partitions. There's a comment in tablecmds.c
> alluding to a possible "UNDER" privilege:
>
>   /*
>    * We should have an UNDER permission flag for this, but for now,
>    * demand that creator of a child table own the parent.
>    */
>
> Perhaps there's something we want to do there, but it's a different use
> case than the MAINTAIN privilege, so I don't see a reason it should be
> grouped. Also, there's a bit of weirdness to think about in cases where
> another user creates (and owns) a partition of your table (currently
> this is only possible if the other user is a superuser).
>

I strongly agree. MAINTAIN is for actions that leave the schema the same.
Conceptually, running MAINTAIN shouldn't affect the result of pg_dump. That
may not be strictly true, but adding a table is definitely not something
that MAINTAIN should allow.

Is there a firm decision on the issue of changing the cluster index of a
table? Re-clustering a table on the same index is clearly something that
should be granted by MAINTAIN as I imagine it, but changing the cluster
index, strictly speaking, changes the schema and could be considered
outside of the scope of what should be allowed. On the other hand, I can
see simplicity in having CLUSTER check the same permissions whether or not
the cluster index is being updated.

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Jeff Davis <pgsql@j-davis.com> — 2022-12-14T20:56:59Z

On Wed, 2022-12-14 at 15:32 -0500, Isaac Morland wrote:

> Is there a firm decision on the issue of changing the cluster index
> of a table? Re-clustering a table on the same index is clearly
> something that should be granted by MAINTAIN as I imagine it, but
> changing the cluster index, strictly speaking, changes the schema and
> could be considered outside of the scope of what should be allowed.
> On the other hand, I can see simplicity in having CLUSTER check the
> same permissions whether or not the cluster index is being updated.

In both the case of CLUSTER and REFRESH, I don't have a strong enough
opinion to break them out into separate privileges. There's some
argument that can be made; but at the same time it's hard for me to
imagine someone really making use of the privileges separately.

-- 
Jeff Davis
PostgreSQL Contributor Team - AWS

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Pavel Luzanov <p.luzanov@postgrespro.ru> — 2022-12-14T22:02:39Z

On 14.12.2022 22:46, Jeff Davis wrote:
> The behavior is that MAINTAIN
> privileges on the partitioned table does not imply MAINTAIN privileges
> on the partitions. I believe that's fine and it's consistent with other
> privileges on partitioned tables, such as SELECT and INSERT.

Sorry, I may have missed something, but here's what I see:

postgres@postgres(16.0)=# create table p (id int) partition by list (id);
postgres@postgres(16.0)=# create table p1 partition of p for values in (1);
postgres@postgres(16.0)=# create table p2 partition of p for values in (2);

postgres@postgres(16.0)=# grant select, insert, maintain on p to alice ;

postgres@postgres(16.0)=# \c - alice
You are now connected to database "postgres" as user "alice".

alice@postgres(16.0)=> insert into p values (1);
INSERT 0 1
alice@postgres(16.0)=> select * from p;
  id
----
   1
(1 row)

alice@postgres(16.0)=> vacuum p;
WARNING:  permission denied to vacuum "p1", skipping it
WARNING:  permission denied to vacuum "p2", skipping it
VACUUM

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Justin Pryzby <pryzby@telsasoft.com> — 2022-12-14T22:11:40Z

On Thu, Dec 15, 2022 at 01:02:39AM +0300, Pavel Luzanov wrote:
> On 14.12.2022 22:46, Jeff Davis wrote:
> > The behavior is that MAINTAIN
> > privileges on the partitioned table does not imply MAINTAIN privileges
> > on the partitions. I believe that's fine and it's consistent with other
> > privileges on partitioned tables, such as SELECT and INSERT.
> 
> Sorry, I may have missed something, but here's what I see:
> 
> postgres@postgres(16.0)=# create table p (id int) partition by list (id);
> postgres@postgres(16.0)=# create table p1 partition of p for values in (1);
> postgres@postgres(16.0)=# create table p2 partition of p for values in (2);
> 
> postgres@postgres(16.0)=# grant select, insert, maintain on p to alice ;
> 
> postgres@postgres(16.0)=# \c - alice
> You are now connected to database "postgres" as user "alice".
> 
> alice@postgres(16.0)=> insert into p values (1);
> INSERT 0 1
> alice@postgres(16.0)=> select * from p;
>  id
> ----
>   1
> (1 row)
> 
> alice@postgres(16.0)=> vacuum p;
> WARNING:  permission denied to vacuum "p1", skipping it
> WARNING:  permission denied to vacuum "p2", skipping it
> VACUUM

Yeah, but:

regression=> insert into p1 values (1);
ERROR:  permission denied for table p1
regression=> select * from p1;
ERROR:  permission denied for table p1

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Isaac Morland <isaac.morland@gmail.com> — 2022-12-14T22:40:14Z

On Wed, 14 Dec 2022 at 15:57, Jeff Davis <pgsql@j-davis.com> wrote:

> On Wed, 2022-12-14 at 15:32 -0500, Isaac Morland wrote:
>
> > Is there a firm decision on the issue of changing the cluster index
> > of a table? Re-clustering a table on the same index is clearly
> > something that should be granted by MAINTAIN as I imagine it, but
> > changing the cluster index, strictly speaking, changes the schema and
> > could be considered outside of the scope of what should be allowed.
> > On the other hand, I can see simplicity in having CLUSTER check the
> > same permissions whether or not the cluster index is being updated.
>
> In both the case of CLUSTER and REFRESH, I don't have a strong enough
> opinion to break them out into separate privileges. There's some
> argument that can be made; but at the same time it's hard for me to
> imagine someone really making use of the privileges separately.
>

Thanks, that makes a lot of sense. I wanted to make sure the question was
considered. I'm very pleased this is happening and appreciate all the work
you're doing. I have a few places where I want to be able to grant MAINTAIN
so I'll be using this as soon as it's available on our production database.

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2022-12-14T23:29:39Z

On Wed, Dec 14, 2022 at 11:05:13AM -0800, Jeff Davis wrote:
> On Wed, 2022-12-14 at 10:16 -0800, Nathan Bossart wrote:
>> Okay.  Should all the privileges governed by MAINTAIN apply to a
>> relation's
>> TOAST table as well?
> 
> Yes, I agree.

This might be tricky, because AFAICT you have to scan pg_class to find a
TOAST table's main relation.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Michael Paquier <michael@paquier.xyz> — 2022-12-15T00:12:26Z

On Wed, Dec 14, 2022 at 03:29:39PM -0800, Nathan Bossart wrote:
> On Wed, Dec 14, 2022 at 11:05:13AM -0800, Jeff Davis wrote:
>> On Wed, 2022-12-14 at 10:16 -0800, Nathan Bossart wrote:
>>> Okay.  Should all the privileges governed by MAINTAIN apply to a
>>> relation's
>>> TOAST table as well?
>> 
>> Yes, I agree.
> 
> This might be tricky, because AFAICT you have to scan pg_class to find a
> TOAST table's main relation.

Ugh, yeah.  Are we talking about a case where we know the toast
information but need to look back at some information of its parent to
do a decision?  I don't recall a case where we do that.  CLUSTER,
REINDEX and VACUUM lock first the parent when working on it, and no
AEL is taken on the parent if doing directly a VACUUM or a REINDEX on
the toast table, so that could lead to deadlock scenarios.  Shouldn't
MAINTAIN be sent down to the toast table as well if that's not done
this way?

FWIW, I have briefly poked at that here:
https://www.postgresql.org/message-id/YZI+aNEnnpBASxNU@paquier.xyz
--
Michael

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Jeff Davis <pgsql@j-davis.com> — 2022-12-15T00:18:05Z

On Wed, 2022-12-14 at 16:11 -0600, Justin Pryzby wrote:
> Yeah, but:
> 
> regression=> insert into p1 values (1);
> ERROR:  permission denied for table p1
> regression=> select * from p1;
> ERROR:  permission denied for table p1

Right, that's what I had in mind: a user is only granted operations on
the partitioned table, not the partitions.

It happens that an INSERT or SELECT on the partitioned table flows
through to the partitions, whereas the VACUUM ends up skipping them, so
I guess the analogy could be interpreted either way. Hmmm...

Thinking about it another way: logical partitioning is about making the
table logically one table, but physically many tables. That would imply
that the privileges should apply per-partition. But then that doesn't
make a lot of sense, because what maintenance can you do on the
partitioned table (which itself has no data)?

There's definitely a problem with this patch and partitioning, because
REINDEX affects the partitions, CLUSTER is a no-op, and VACUUM/ANALYZE
skip them.

-- 
Jeff Davis
PostgreSQL Contributor Team - AWS

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2022-12-15T00:27:05Z

On Thu, Dec 15, 2022 at 09:12:26AM +0900, Michael Paquier wrote:
> On Wed, Dec 14, 2022 at 03:29:39PM -0800, Nathan Bossart wrote:
>> On Wed, Dec 14, 2022 at 11:05:13AM -0800, Jeff Davis wrote:
>>> On Wed, 2022-12-14 at 10:16 -0800, Nathan Bossart wrote:
>>>> Okay.  Should all the privileges governed by MAINTAIN apply to a
>>>> relation's
>>>> TOAST table as well?
>>> 
>>> Yes, I agree.
>> 
>> This might be tricky, because AFAICT you have to scan pg_class to find a
>> TOAST table's main relation.
> 
> Ugh, yeah.  Are we talking about a case where we know the toast
> information but need to look back at some information of its parent to
> do a decision?  I don't recall a case where we do that.  CLUSTER,
> REINDEX and VACUUM lock first the parent when working on it, and no
> AEL is taken on the parent if doing directly a VACUUM or a REINDEX on
> the toast table, so that could lead to deadlock scenarios.  Shouldn't
> MAINTAIN be sent down to the toast table as well if that's not done
> this way?

Another option I'm looking at is skipping the privilege checks when VACUUM
recurses to a TOAST table.  This won't allow you to VACUUM the TOAST table
directly, but it would at least address the originally-reported issue [0].

Since you can't ANALYZE, REFRESH, or LOCK TOAST tables, this isn't a
problem for those commands.  CLUSTER and REINDEX seem to process relations'
TOAST tables without extra privilege checks already.  So with the attached
patch applied, you wouldn't be able to VACUUM, CLUSTER, and REINDEX TOAST
tableѕ directly (unless you were given MAINTAIN or pg_maintain), but you
could indirectly process them by specifying the main relation.

I don't know if this is good enough.  It seems like ideally you should be
able to VACUUM a TOAST table directly if you have MAINTAIN on its main
relation.

[0] https://postgr.es/m/b572d238-0de2-9cad-5f34-4741dc627834%40postgrespro.ru

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Pavel Luzanov <p.luzanov@postgrespro.ru> — 2022-12-15T09:31:00Z

On 15.12.2022 03:18, Jeff Davis wrote:
> Right, that's what I had in mind: a user is only granted operations on
> the partitioned table, not the partitions.

It's all clear now.

> There's definitely a problem with this patch and partitioning, because
> REINDEX affects the partitions, CLUSTER is a no-op, and VACUUM/ANALYZE
> skip them.

I think the approach that Nathan implemented [1] for TOAST tables
in the latest version can be used for partitioned tables as well.
Skipping the privilege check for partitions while working with
a partitioned table. In that case we would get exactly the same behavior
as for INSERT, SELECT, etc privileges - the MAINTAIN privilege would 
work for
the whole partitioned table, but not for individual partitions.

[1] 
https://www.postgresql.org/message-id/20221215002705.GA889413%40nathanxps13

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Pavel Luzanov <p.luzanov@postgrespro.ru> — 2022-12-15T09:42:00Z

On 15.12.2022 03:27, Nathan Bossart wrote:
> Another option I'm looking at is skipping the privilege checks when VACUUM
> recurses to a TOAST table.  This won't allow you to VACUUM the TOAST table
> directly, but it would at least address the originally-reported issue

This approach can be implemented for partitioned tables too. Skipping
the privilege checks when VACUUM/ANALYZE recurses to partitions.

> I don't know if this is good enough.

At least it's better than before.

> It seems like ideally you should be
> able to VACUUM a TOAST table directly if you have MAINTAIN on its main
> relation.

I agree, that would be ideally.

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Jeff Davis <pgsql@j-davis.com> — 2022-12-15T18:10:43Z

On Thu, 2022-12-15 at 12:31 +0300, Pavel Luzanov wrote:
> I think the approach that Nathan implemented [1] for TOAST tables
> in the latest version can be used for partitioned tables as well.
> Skipping the privilege check for partitions while working with
> a partitioned table. In that case we would get exactly the same
> behavior
> as for INSERT, SELECT, etc privileges - the MAINTAIN privilege would 
> work for
> the whole partitioned table, but not for individual partitions.

There is some weirdness in 15, too:

   create user foo;
   create user su superuser;
   grant all privileges on schema public to foo;

   \c - foo
   create table p(i int) partition by range (i);
   create index p_idx on p (i);
   create table p0 partition of p for values from (0) to (10);

   \c - su
   create table p1 partition of p for values from (10) to (20);

   \c - foo

   -- possibly weird because the 15 inserts into p1 (owned by su)
   insert into p values (5), (15);

   -- all these are as expected:
   select * from p; -- returns 5 & 15
   insert into p1 values(16); -- permission denied
   select * from p1; -- permission denied
   
   -- the following commands seem inconsistent to me:
   vacuum p; -- skips p1 with warning
   analyze p; -- skips p1 with warning
   cluster p using p_idx; -- silently skips p1
   reindex table p; -- reindexes p0 and p1 (owned by su)

   -- RLS is also bypassed
   \c - su
   grant select, insert on p1 to foo;
   alter table p1 enable row level security;
   create policy odd on p1 using (i % 2 = 1) with check (i % 2 = 1);
   \c - foo
   insert into p1 values (16); -- RLS error
   insert into p values (16); -- succeeds
   select * from p1; -- returns only 15
   select * from p; -- returns 5, 15, 16
   
The proposal to skip privilege checks for partitions would be
consistent with INSERT, SELECT, REINDEX that flow through to the
underlying partitions regardless of permissions/ownership (and even
RLS). It would be very minor behavior change on 15 for this weird case
of superuser-owned partitions, but I doubt anyone would be relying on
that.

+1.

I do have some lingering doubts about whether we should even allow
inconsistent ownership/permissions. But I don't think we need to settle
that question now.


-- 
Jeff Davis
PostgreSQL Contributor Team - AWS

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Jeff Davis <pgsql@j-davis.com> — 2022-12-15T18:42:15Z

On Wed, 2022-12-14 at 16:27 -0800, Nathan Bossart wrote:
> I don't know if this is good enough.  It seems like ideally you
> should be
> able to VACUUM a TOAST table directly if you have MAINTAIN on its
> main
> relation.

Right now, targetting the toast table directly requires the USAGE
privilege on the toast schema, and you have to look up the name first,
right? As it is, that's not a great UI.

How about if we add a VACUUM option like TOAST_ONLY (or combine it with
the PROCESS_TOAST option)? Then, you're always looking at the parent
table first so there's no deadlock, do the permission checks on the
parent, and then expand to the toast table with no check. This can be a
follow-up patch; for now, the idea of skipping the privilege checks
when expanding looks like an improvement.

-- 
Jeff Davis
PostgreSQL Contributor Team - AWS

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2022-12-15T19:12:46Z

On Thu, Dec 15, 2022 at 10:42:15AM -0800, Jeff Davis wrote:
> Right now, targetting the toast table directly requires the USAGE
> privilege on the toast schema, and you have to look up the name first,
> right? As it is, that's not a great UI.
> 
> How about if we add a VACUUM option like TOAST_ONLY (or combine it with
> the PROCESS_TOAST option)? Then, you're always looking at the parent
> table first so there's no deadlock, do the permission checks on the
> parent, and then expand to the toast table with no check. This can be a
> follow-up patch; for now, the idea of skipping the privilege checks
> when expanding looks like an improvement.

I originally suggested an option to allow specifying whether to process the
main relation, but we ended up only adding PROCESS_TOAST [0].  FWIW I still
think that such an option would be useful for the reasons you describe.

[0] https://postgr.es/m/BA8951E9-1524-48C5-94AF-73B1F0D7857F%40amazon.com

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Justin Pryzby <pryzby@telsasoft.com> — 2022-12-15T19:48:13Z

On Thu, Dec 15, 2022 at 10:10:43AM -0800, Jeff Davis wrote:
> On Thu, 2022-12-15 at 12:31 +0300, Pavel Luzanov wrote:
> > I think the approach that Nathan implemented [1] for TOAST tables
> > in the latest version can be used for partitioned tables as well.
> > Skipping the privilege check for partitions while working with
> > a partitioned table. In that case we would get exactly the same
> > behavior
> > as for INSERT, SELECT, etc privileges - the MAINTAIN privilege would 
> > work for
> > the whole partitioned table, but not for individual partitions.
> 
> There is some weirdness in 15, too:

I gather you mean postgresql v15.1 and master ?

>    -- the following commands seem inconsistent to me:
>    vacuum p; -- skips p1 with warning
>    analyze p; -- skips p1 with warning
>    cluster p using p_idx; -- silently skips p1
>    reindex table p; -- reindexes p0 and p1 (owned by su)

Clustering on a partitioned table is new in v15, and this behavior is
from 3f19e176ae0 and cfdd03f45e6, which added
get_tables_to_cluster_partitioned(), borrowing from expand_vacuum_rel()
and get_tables_to_cluster().

vacuum initially calls vacuum_is_permitted_for_relation() only for the
partitioned table, and *later* locks the partition and then checks its
permissions, which is when the message is output.

Since v15, cluster calls get_tables_to_cluster_partitioned(), which
silently discards partitions failing ACL.

We could change it to emit a message, which would seem to behave like
vacuum, except that the check is happening earlier, and (unlike vacuum)
partitions skipped later during CLUOPT_RECHECK wouldn't have any message
output.

Or we could change cluster_rel() to output a message when skipping.  But
these patches hardly touched that function at all.  I suppose we could
change to emit a message during RECHECK (maybe only in master branch).
If need be, that could be controlled by a new CLUOPT_*.

-- 
Justin

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2022-12-16T01:19:26Z

On Thu, Dec 15, 2022 at 01:48:13PM -0600, Justin Pryzby wrote:
> vacuum initially calls vacuum_is_permitted_for_relation() only for the
> partitioned table, and *later* locks the partition and then checks its
> permissions, which is when the message is output.
> 
> Since v15, cluster calls get_tables_to_cluster_partitioned(), which
> silently discards partitions failing ACL.
> 
> We could change it to emit a message, which would seem to behave like
> vacuum, except that the check is happening earlier, and (unlike vacuum)
> partitions skipped later during CLUOPT_RECHECK wouldn't have any message
> output.
> 
> Or we could change cluster_rel() to output a message when skipping.  But
> these patches hardly touched that function at all.  I suppose we could
> change to emit a message during RECHECK (maybe only in master branch).
> If need be, that could be controlled by a new CLUOPT_*.

Yeah, VACUUM/ANALYZE works differently.  For one, you can specify multiple
relations in the command.  Also, VACUUM/ANALYZE only takes an
AccessShareLock when first assessing permissions (which actually skips
partitions).  CLUSTER immediately takes an AccessExclusiveLock, so the
permissions are checked up front.  This is done "to avoid lock-upgrade
hazard in the single-transaction case," which IIUC is what allows CLUSTER
on a single table to run within a transaction block (unlike VACUUM).  I
don't know if running CLUSTER in a transaction block is important.  IMO we
should consider making CLUSTER look a lot more like VACUUM/ANALYZE in this
regard.  The attached patch adds WARNING messages, but we're still far from
consistency with VACUUM/ANALYZE.

Side note: I see that CLUSTER on a partitioned table is disallowed in a
transaction block, which should probably be added to my documentation patch
[0].

[0] https://commitfest.postgresql.org/41/4054/

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Jeff Davis <pgsql@j-davis.com> — 2022-12-16T04:35:53Z

On Thu, 2022-12-15 at 17:19 -0800, Nathan Bossart wrote:
> The attached patch adds WARNING messages, but we're still far from
> consistency with VACUUM/ANALYZE.

But why make CLUSTER more like VACUUM? Shouldn't we make
VACUUM/CLUSTER/ANALYZE more like INSERT/SELECT/REINDEX?

As far as I can tell, the only way you can get in this situation in 15
is by having a superuser create partitions in a non-superuser's table.
I don't think that was an intended use case, so it's probably just
historical reasons that VACUUM/CLUSTER/ANALYZE ended up that way, not
precedent we should necessarily follow.

-- 
Jeff Davis
PostgreSQL Contributor Team - AWS

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2022-12-16T05:13:54Z

On Thu, Dec 15, 2022 at 08:35:53PM -0800, Jeff Davis wrote:
> But why make CLUSTER more like VACUUM? Shouldn't we make
> VACUUM/CLUSTER/ANALYZE more like INSERT/SELECT/REINDEX?

Hm.  Since VACUUM may happen across multiple transactions, it is careful to
re-check the privileges for each relation.  For example, vacuum_rel()
contains this comment:

	/*
	 * Check if relation needs to be skipped based on privileges.  This check
	 * happens also when building the relation list to vacuum for a manual
	 * operation, and needs to be done additionally here as VACUUM could
	 * happen across multiple transactions where privileges could have changed
	 * in-between.  Make sure to only generate logs for VACUUM in this case.
	 */

I do wonder whether this is something we really need to be concerned about.
In the worst case, you might be able to VACUUM a table with a concurrent
owner change.

The logic for gathering all relations to process (i.e.,
get_all_vacuum_rels() and get_tables_to_cluster()) would also need to be
adjusted to handle partitioned tables.  Right now, we gather all the
partitions and checks privileges on each.  I think this would be easy to
change.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2022-12-16T05:20:21Z

On Thu, Dec 15, 2022 at 09:13:54PM -0800, Nathan Bossart wrote:
> I do wonder whether this is something we really need to be concerned about.
> In the worst case, you might be able to VACUUM a table with a concurrent
> owner change.

I suppose we might want to avoid running the index functions as the new
owner.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2022-12-17T06:04:08Z

On Thu, Dec 15, 2022 at 10:10:43AM -0800, Jeff Davis wrote:
> The proposal to skip privilege checks for partitions would be
> consistent with INSERT, SELECT, REINDEX that flow through to the
> underlying partitions regardless of permissions/ownership (and even
> RLS). It would be very minor behavior change on 15 for this weird case
> of superuser-owned partitions, but I doubt anyone would be relying on
> that.

I've attached a work-in-progress patch that aims to accomplish this.
Instead of skipping the privilege checks, I added logic to trawl through
pg_inherits and pg_class to check whether the user has privileges for the
partitioned table or for the main relation of a TOAST table.  This means
that MAINTAIN on a partitioned table is enough to execute maintenance
commands on all the partitions, and MAINTAIN on a main relation is enough
to execute maintenance commands on its TOAST table.  Also, the maintenance
commands that flow through to the partitions or the TOAST table should no
longer error due to permissions when the user only has MAINTAIN on the
paritioned table or main relation.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Ted Yu <yuzhihong@gmail.com> — 2022-12-17T12:39:29Z

On Fri, Dec 16, 2022 at 10:04 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:

> On Thu, Dec 15, 2022 at 10:10:43AM -0800, Jeff Davis wrote:
> > The proposal to skip privilege checks for partitions would be
> > consistent with INSERT, SELECT, REINDEX that flow through to the
> > underlying partitions regardless of permissions/ownership (and even
> > RLS). It would be very minor behavior change on 15 for this weird case
> > of superuser-owned partitions, but I doubt anyone would be relying on
> > that.
>
> I've attached a work-in-progress patch that aims to accomplish this.
> Instead of skipping the privilege checks, I added logic to trawl through
> pg_inherits and pg_class to check whether the user has privileges for the
> partitioned table or for the main relation of a TOAST table.  This means
> that MAINTAIN on a partitioned table is enough to execute maintenance
> commands on all the partitions, and MAINTAIN on a main relation is enough
> to execute maintenance commands on its TOAST table.  Also, the maintenance
> commands that flow through to the partitions or the TOAST table should no
> longer error due to permissions when the user only has MAINTAIN on the
> paritioned table or main relation.
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com

Hi,

+cluster_is_permitted_for_relation(Oid relid, Oid userid)
+{
+       return pg_class_aclcheck(relid, userid, ACL_MAINTAIN) ==
ACLCHECK_OK ||
+                  has_parent_privs(relid, userid, ACL_MAINTAIN);

Since the func only contains one statement, it seems this can be defined as
a macro instead.

+       List       *ancestors = get_partition_ancestors(relid);
+       Oid                     root = InvalidOid;

nit: it would be better if the variable `root` can be aligned with variable
`ancestors`.

Cheers

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2022-12-18T23:30:18Z

Here is a new version of the patch.  Besides some cleanup, I added an index
on reltoastrelid for the toast-to-main-relation lookup.  Before I bother
adjusting the tests and documentation, I'm curious to hear thoughts on
whether this seems like a viable approach.

On Sat, Dec 17, 2022 at 04:39:29AM -0800, Ted Yu wrote:
> +cluster_is_permitted_for_relation(Oid relid, Oid userid)
> +{
> +       return pg_class_aclcheck(relid, userid, ACL_MAINTAIN) ==
> ACLCHECK_OK ||
> +                  has_parent_privs(relid, userid, ACL_MAINTAIN);
> 
> Since the func only contains one statement, it seems this can be defined as
> a macro instead.

In the new version, there is a bit more to this function, so I didn't
convert it to a macro.

> +       List       *ancestors = get_partition_ancestors(relid);
> +       Oid                     root = InvalidOid;
> 
> nit: it would be better if the variable `root` can be aligned with variable
> `ancestors`.

Hm.  It looked alright on my machine.  In any case, I'll be sure to run
pgindent at some point.  This patch is still in early stages.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Justin Pryzby <pryzby@telsasoft.com> — 2022-12-18T23:38:03Z

On Sat, Dec 17, 2022 at 04:39:29AM -0800, Ted Yu wrote:
> +       List       *ancestors = get_partition_ancestors(relid);
> +       Oid                     root = InvalidOid;
> 
> nit: it would be better if the variable `root` can be aligned with variable
> `ancestors`.

It is aligned, but only after configuring one's editor/pager/mail client
to display tabs in the manner assumed by postgres' coding style.

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Ted Yu <yuzhihong@gmail.com> — 2022-12-19T00:25:15Z

On Sun, Dec 18, 2022 at 3:30 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:

> Here is a new version of the patch.  Besides some cleanup, I added an index
> on reltoastrelid for the toast-to-main-relation lookup.  Before I bother
> adjusting the tests and documentation, I'm curious to hear thoughts on
> whether this seems like a viable approach.
>
> On Sat, Dec 17, 2022 at 04:39:29AM -0800, Ted Yu wrote:
> > +cluster_is_permitted_for_relation(Oid relid, Oid userid)
> > +{
> > +       return pg_class_aclcheck(relid, userid, ACL_MAINTAIN) ==
> > ACLCHECK_OK ||
> > +                  has_parent_privs(relid, userid, ACL_MAINTAIN);
> >
> > Since the func only contains one statement, it seems this can be defined
> as
> > a macro instead.
>
> In the new version, there is a bit more to this function, so I didn't
> convert it to a macro.
>
> > +       List       *ancestors = get_partition_ancestors(relid);
> > +       Oid                     root = InvalidOid;
> >
> > nit: it would be better if the variable `root` can be aligned with
> variable
> > `ancestors`.
>
> Hm.  It looked alright on my machine.  In any case, I'll be sure to run
> pgindent at some point.  This patch is still in early stages.
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com

Hi,

+ * Note: Because this function assumes that the realtion whose OID is
passed as

Typo:  realtion -> relation

Cheers

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2022-12-19T00:31:35Z

On Sun, Dec 18, 2022 at 04:25:15PM -0800, Ted Yu wrote:
> + * Note: Because this function assumes that the realtion whose OID is
> passed as
> 
> Typo:  realtion -> relation

Thanks, fixed.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Jeff Davis <pgsql@j-davis.com> — 2022-12-19T22:43:05Z

On Sun, 2022-12-18 at 17:38 -0600, Justin Pryzby wrote:
> On Sat, Dec 17, 2022 at 04:39:29AM -0800, Ted Yu wrote:
> > +       List       *ancestors = get_partition_ancestors(relid);
> > +       Oid                     root = InvalidOid;
> > 
> > nit: it would be better if the variable `root` can be aligned with
> > variable
> > `ancestors`.
> 
> It is aligned, but only after configuring one's editor/pager/mail
> client
> to display tabs in the manner assumed by postgres' coding style.

If you use emacs or vim, there are editor config samples in
src/tools/editors/

Regards,
	Jeff Davis

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2023-01-03T23:45:49Z

On Sun, Dec 18, 2022 at 03:30:18PM -0800, Nathan Bossart wrote:
> Here is a new version of the patch.  Besides some cleanup, I added an index
> on reltoastrelid for the toast-to-main-relation lookup.  Before I bother
> adjusting the tests and documentation, I'm curious to hear thoughts on
> whether this seems like a viable approach.

I'd like to get this fixed, but I have yet to hear thoughts on the
suggested approach.  I'll proceed with adjusting the tests and
documentation shortly unless someone objects.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2023-01-09T22:51:57Z

On Tue, Jan 03, 2023 at 03:45:49PM -0800, Nathan Bossart wrote:
> I'd like to get this fixed, but I have yet to hear thoughts on the
> suggested approach.  I'll proceed with adjusting the tests and
> documentation shortly unless someone objects.

As promised, here is a new version of the patch with adjusted tests and
documentation.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Jeff Davis <pgsql@j-davis.com> — 2023-01-13T19:56:03Z

On Mon, 2023-01-09 at 14:51 -0800, Nathan Bossart wrote:
> On Tue, Jan 03, 2023 at 03:45:49PM -0800, Nathan Bossart wrote:
> > I'd like to get this fixed, but I have yet to hear thoughts on the
> > suggested approach.  I'll proceed with adjusting the tests and
> > documentation shortly unless someone objects.
> 
> As promised, here is a new version of the patch with adjusted tests
> and
> documentation.

The current patch doesn't handle the case properly where you have
partitions that have toast tables. An easy fix by recursing, but I
think we might want to do things differently anyway.

I'm hesitant to add an index to pg_class just for the privilege checks
on toast tables, and I don't think we need to. Instead, we can just
skip the privilege check on a toast table if it's not referenced
directly, because we already checked the privileges on the parent, and
we still hold the session lock so nothing strange should have happened.

I'll look into that, but so far it looks like it should come out
cleanly for toast tables. The way you're checking privileges on the
partitioned tables is fine.

-- 
Jeff Davis
PostgreSQL Contributor Team - AWS

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2023-01-13T20:33:34Z

On Fri, Jan 13, 2023 at 11:56:03AM -0800, Jeff Davis wrote:
> I'm hesitant to add an index to pg_class just for the privilege checks
> on toast tables, and I don't think we need to.

I bet this index will be useful for more than just these privilege checks
(e.g., autovacuum currently creates a hash table for the
toast-to-main-relation mapping), but I do understand the hesitation.

> Instead, we can just
> skip the privilege check on a toast table if it's not referenced
> directly, because we already checked the privileges on the parent, and
> we still hold the session lock so nothing strange should have happened.

That would fix the problem in the original complaint, but it wouldn't allow
for vacuuming toast tables directly if you only have MAINTAIN privileges on
the main relation.  If you can vacuum the toast table indirectly via the
main relation, shouldn't it be possible to vacuum it directly?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Jeff Davis <pgsql@j-davis.com> — 2023-01-13T21:30:28Z

On Fri, 2023-01-13 at 12:33 -0800, Nathan Bossart wrote:
> That would fix the problem in the original complaint, but it wouldn't
> allow
> for vacuuming toast tables directly if you only have MAINTAIN
> privileges on
> the main relation.  If you can vacuum the toast table indirectly via
> the
> main relation, shouldn't it be possible to vacuum it directly?

Perhaps, but that's barely supported today: you have to awkwardly find
the internal toast table name yourself, and you need the admin to grant
you USAGE on the pg_toast schema. I don't think we're obligated to also
support this hackery for non-owners with a new MAINTAIN privilege.

If we care about that use case, let's do it right and have forms of
VACUUM/CLUSTER/REINDEX that check permissions on the main table, skip
the work on the main table, and descend directly to the toast tables.
That doesn't seem hard, but it's a separate patch.

Right now, we should simply fix the problem.

-- 
Jeff Davis
PostgreSQL Contributor Team - AWS

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2023-01-13T22:56:26Z

On Fri, Jan 13, 2023 at 01:30:28PM -0800, Jeff Davis wrote:
> If we care about that use case, let's do it right and have forms of
> VACUUM/CLUSTER/REINDEX that check permissions on the main table, skip
> the work on the main table, and descend directly to the toast tables.
> That doesn't seem hard, but it's a separate patch.

You may be interested in https://commitfest.postgresql.org/41/4088/.

> Right now, we should simply fix the problem.

Okay.  Here is a new patch set.  I've split the partition work out to a
separate patch, and I've removed the main relation lookups for TOAST tables
in favor of adding a skip_privs flag to vacuum_rel().  The latter patch
probably needs some additional commentary and tests, which I'll go ahead
and add if we want to proceed with this approach.  I'm assuming the session
lock should be sufficient for avoiding the case where the TOAST table's OID
is reused by the time we get to it, but I'm not sure if it's sufficient to
prevent vacuuming if the privileges on the main relation have changed
across transactions.  Even if it's not, I'm not sure that case is worth
worrying about too much.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2023-01-13T23:13:39Z

On Fri, Jan 13, 2023 at 02:56:26PM -0800, Nathan Bossart wrote:
> Okay.  Here is a new patch set.

And here is a rebased patch set (c44f633 changed the same LOCK TABLE docs).

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2023-06-13T21:12:46Z

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 some
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.

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).

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.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Michael Paquier <michael@paquier.xyz> — 2023-06-13T23:16:15Z

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 some
> 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).
> 
> 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

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2023-06-13T23:54:42Z

On Wed, Jun 14, 2023 at 08:16:15AM +0900, Michael Paquier wrote:
> 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?

Yeah, that could probably be moved into VacuumParams.

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

Thanks.  I suspect there's more discussion incoming, but I'm hoping to
close this item one way or another by 16beta2.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2023-06-14T18:17:11Z

On Tue, Jun 13, 2023 at 04:54:42PM -0700, Nathan Bossart wrote:
> On Wed, Jun 14, 2023 at 08:16:15AM +0900, Michael Paquier wrote:
>> So, yes, agreed about the removal of has_partition_ancestor_privs().
>> I am adding an open item assigned to you and Jeff.
> 
> Thanks.  I suspect there's more discussion incoming, but I'm hoping to
> close this item one way or another by 16beta2.

Concretely, I am proposing something like the attached patches.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Jeff Davis <pgsql@j-davis.com> — 2023-06-14T18:30:45Z

On Tue, 2023-06-13 at 14:12 -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 some
> 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.

Agreed. Having it work on partition hierarchies is a nice-to-have, but
not central to the usability of the feature. If it's causing problems,
best to take that out and reconsider in 17 if worthwhile.

Regards,
	Jeff Davis

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Michael Paquier <michael@paquier.xyz> — 2023-06-15T00:46:33Z

On Wed, Jun 14, 2023 at 11:17:11AM -0700, Nathan Bossart wrote:
> On Tue, Jun 13, 2023 at 04:54:42PM -0700, Nathan Bossart wrote:
> > On Wed, Jun 14, 2023 at 08:16:15AM +0900, Michael Paquier wrote:
> >> So, yes, agreed about the removal of has_partition_ancestor_privs().
> >> I am adding an open item assigned to you and Jeff.
> > 
> > Thanks.  I suspect there's more discussion incoming, but I'm hoping to
> > close this item one way or another by 16beta2.
> 
> Concretely, I am proposing something like the attached patches.

The result after 0001 is applied is that a couple of
object_ownercheck() calls that existed before ff9618e are removed from
some ACL checks in the REINDEX, CLUSTER and VACUUM paths.  Is that OK
for shared relations and shouldn't cluster_is_permitted_for_relation()
include that?  vacuum_is_permitted_for_relation() is consistent on
this side.

Here are the paths that now differ:
cluster_rel
get_tables_to_cluster
get_tables_to_cluster_partitioned
RangeVarCallbackForReindexIndex
ReindexMultipleTables

0002 looks OK to retain the skip check for toast relations in the
VACUUM case.
--
Michael

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2023-06-15T04:10:44Z

On Thu, Jun 15, 2023 at 09:46:33AM +0900, Michael Paquier wrote:
> The result after 0001 is applied is that a couple of
> object_ownercheck() calls that existed before ff9618e are removed from
> some ACL checks in the REINDEX, CLUSTER and VACUUM paths.  Is that OK
> for shared relations and shouldn't cluster_is_permitted_for_relation()
> include that?  vacuum_is_permitted_for_relation() is consistent on
> this side.

These object_ownercheck() calls were removed because they were redundant,
as owners have all privileges by default.  Privileges can be revoked from
the owner, so an extra ownership check would effectively bypass the
relation's ACL in that case.  I looked around and didn't see any other
examples of a combined ownership and ACL check like we were doing for
MAINTAIN.  The only thing that gives me pause is that the docs call out
ownership as sufficient for some maintenance commands.  With these patches,
that's only true as long as no one revokes privileges from the owner.  IMO
we should update the docs and leave out the ownership checks since MAINTAIN
is now a grantable privilege like any other.  WDYT?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2023-06-15T23:57:00Z

On Wed, Jun 14, 2023 at 09:10:44PM -0700, Nathan Bossart wrote:
> IMO
> we should update the docs and leave out the ownership checks since MAINTAIN
> is now a grantable privilege like any other.  WDYT?

Here's an attempt at adjusting the documentation as I proposed yesterday.
I think this is a good opportunity to simplify the privilege-related
sections for these maintenance commands.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2023-06-16T05:20:25Z

On Thu, Jun 15, 2023 at 04:57:00PM -0700, Nathan Bossart wrote:
> Here's an attempt at adjusting the documentation as I proposed yesterday.
> I think this is a good opportunity to simplify the privilege-related
> sections for these maintenance commands.

I noticed that it was possible to make the documentation changes in 0001
easier to read.  Apologies for the noise.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2023-06-19T21:55:34Z

On Thu, Jun 15, 2023 at 10:20:25PM -0700, Nathan Bossart wrote:
> I noticed that it was possible to make the documentation changes in 0001
> easier to read.  Apologies for the noise.

Yet more noise...

In v4 of the patch set, I moved the skip_privs flag refactoring to 0001.  I
intend to commit this tomorrow unless there is additional feedback.

I split out the documentation simplifications to 0003 since it seemed
independent.  Also, I adjusted some ACL-related error messages in 0002 to
appropriately use "permission denied" messages instead of "must be owner"
messages.  I'm hoping to commit 0002 and 0003 by the end of the week so
that these fixes are available in 16beta2.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Michael Paquier <michael@paquier.xyz> — 2023-06-20T05:26:42Z

On Mon, Jun 19, 2023 at 02:55:34PM -0700, Nathan Bossart wrote:
> In v4 of the patch set, I moved the skip_privs flag refactoring to 0001.  I
> intend to commit this tomorrow unless there is additional feedback.

Fine by me.  0001 looks OK seen from here.

> These object_ownercheck() calls were removed because they were redundant,
> as owners have all privileges by default.  Privileges can be revoked from
> the owner, so an extra ownership check would effectively bypass the
> relation's ACL in that case.  I looked around and didn't see any other
> examples of a combined ownership and ACL check like we were doing for
> MAINTAIN.  The only thing that gives me pause is that the docs call out
> ownership as sufficient for some maintenance commands.  With these patches,
> that's only true as long as no one revokes privileges from the owner.  IMO
> we should update the docs and leave out the ownership checks since MAINTAIN
> is now a grantable privilege like any other.  WDYT?

TBH, I have a mixed feeling about this line of reasoning because
MAINTAIN is much broader and less specific than TRUNCATE, for
instance, being spawned across so much more operations.  As you say,
owners of a relation have the MAINTAIN right by default, but they
would not be able to run any maintenance operations if somebody has
revoked their MAINTAIN right to do so, even if they are the owners of
the so-said relation.  Perhaps that's OK in the long run, still I have
mixed feeling about whether that's the best decision we can take here, 
especially because MAINTAIN impacts VACUUM, ANALYZE, CLUSTER, REFRESH
MATVIEW, REINDEX and LOCK.  Some users may find that surprising as they
used to have more control over these operations as owners of the
relations worked on.
--
Michael

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Jeff Davis <pgsql@j-davis.com> — 2023-06-20T16:16:59Z

On Tue, 2023-06-20 at 14:26 +0900, Michael Paquier wrote:
> TBH, I have a mixed feeling about this line of reasoning because
> MAINTAIN is much broader and less specific than TRUNCATE, for
> instance, being spawned across so much more operations.

...

> Some users may find that surprising as they
> used to have more control over these operations as owners of the
> relations worked on.

It seems like the user shouldn't be surprised if they can carry out the
action; nor should they be surprised if they can't carry out the
action. Having privileges revoked on a table from the table's owner is
an edge case in behavior and both make sense to me.

In the absense of a use case, I'd be inclined towards just being
consistent with the other privileges.

Regards,
	Jeff Davis

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Jeff Davis <pgsql@j-davis.com> — 2023-06-20T17:04:37Z

On Mon, 2023-06-19 at 14:55 -0700, Nathan Bossart wrote:
> In v4 of the patch set, I moved the skip_privs flag refactoring to
> 0001.  I
> intend to commit this tomorrow unless there is additional feedback.

I think v4-0001 broke the handling of toast tables? It looks like you
removed the check for !skip_privs but need to add it to the flags in
vacuum_is_permitted_for_relation(). 

Regards,
	Jeff Davis

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2023-06-20T17:40:32Z

On Tue, Jun 20, 2023 at 10:04:37AM -0700, Jeff Davis wrote:
> I think v4-0001 broke the handling of toast tables? It looks like you
> removed the check for !skip_privs but need to add it to the flags in
> vacuum_is_permitted_for_relation(). 

Good catch.  I'm not sure why some of the calls to
vacuum_is_permitted_for_relation() are masking the options.  AFAICT we can
simply remove the masks.  I've done so in the attached patch.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2023-06-20T17:42:10Z

On Tue, Jun 20, 2023 at 09:16:59AM -0700, Jeff Davis wrote:
> On Tue, 2023-06-20 at 14:26 +0900, Michael Paquier wrote:
>> TBH, I have a mixed feeling about this line of reasoning because
>> MAINTAIN is much broader and less specific than TRUNCATE, for
>> instance, being spawned across so much more operations.
> 
> ...
> 
>> Some users may find that surprising as they
>> used to have more control over these operations as owners of the
>> relations worked on.
> 
> It seems like the user shouldn't be surprised if they can carry out the
> action; nor should they be surprised if they can't carry out the
> action. Having privileges revoked on a table from the table's owner is
> an edge case in behavior and both make sense to me.
> 
> In the absense of a use case, I'd be inclined towards just being
> consistent with the other privileges.

Agreed, I think we should make MAINTAIN consistent with the other grantable
privileges.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2023-06-20T17:49:27Z

On Tue, Jun 20, 2023 at 10:40:32AM -0700, Nathan Bossart wrote:
> On Tue, Jun 20, 2023 at 10:04:37AM -0700, Jeff Davis wrote:
>> I think v4-0001 broke the handling of toast tables? It looks like you
>> removed the check for !skip_privs but need to add it to the flags in
>> vacuum_is_permitted_for_relation(). 
> 
> Good catch.  I'm not sure why some of the calls to
> vacuum_is_permitted_for_relation() are masking the options.  AFAICT we can
> simply remove the masks.  I've done so in the attached patch.

Oh, I think I see why.  This appears to be used to control which WARNING
message is emitted.  If you lose permissions before you get to analyzing in
a VACUUM (ANALYZE) command, you'll get a "permission denied to vacuum"
message instead of a "permission denied to analyze" message.  IMO a better
way to do that would be to control only those two bits (VACOPT_VACUUM and
VACOPT_ANALYZE) in calls to vacuum_is_permitted_for_relation(), and to
leave the rest untouched.

Patch incoming...

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2023-06-20T17:56:34Z

On Tue, Jun 20, 2023 at 10:49:27AM -0700, Nathan Bossart wrote:
> Patch incoming...

Attached.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Jeff Davis <pgsql@j-davis.com> — 2023-06-20T18:43:05Z

On Mon, 2023-06-19 at 14:55 -0700, Nathan Bossart wrote:
> I'm hoping to commit 0002 and 0003 by the end of the week so
> that these fixes are available in 16beta2.

A few observations for the case where a user does have the MAINTAIN
privilege on a partitioned table but not the partitions:

 * they can LOCK TABLE on the partitioned table
 * ANALYZE works on the inheritance tree but not the individual
partitions
 * CLUSTER and VACUUM are useless because they skip all of the
partitions. That's consistent with the purpose of this thread -- to
avoid the locking problems trying to support those operations on
partitioned tables.
 * REINDEX TABLE applies to all indexes in all partitions, which seems
a bit inconsistent.

The only behavior I'm worried about is REINDEX. I'm not sure what we
should do about it, or if we even want to do something about it. If we
want REINDEX to fail in this case, we should be sure to check
permissions on everything up-front to avoid doing a lot of work. The
only other option I can think of is to REINDEX only those indexes
declared on the partitioned table (not the individual partitions),
which seems consistent but might be confusing to users.

Regards,
	Jeff Davis

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Jeff Davis <pgsql@j-davis.com> — 2023-06-20T18:49:36Z

On Tue, 2023-06-20 at 10:56 -0700, Nathan Bossart wrote:
> On Tue, Jun 20, 2023 at 10:49:27AM -0700, Nathan Bossart wrote:
> > Patch incoming...
> 
> Attached.

Looks good to me.

Regards,
	Jeff Davis

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2023-06-20T22:20:24Z

On Tue, Jun 20, 2023 at 11:49:36AM -0700, Jeff Davis wrote:
> On Tue, 2023-06-20 at 10:56 -0700, Nathan Bossart wrote:
>> On Tue, Jun 20, 2023 at 10:49:27AM -0700, Nathan Bossart wrote:
>> > Patch incoming...
>> 
>> Attached.
> 
> Looks good to me.

Thanks, committed.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2023-06-20T22:52:57Z

I've attached rebased versions of the remaining two patches.

On Tue, Jun 20, 2023 at 11:43:05AM -0700, Jeff Davis wrote:
>  * REINDEX TABLE applies to all indexes in all partitions, which seems
> a bit inconsistent.
> 
> The only behavior I'm worried about is REINDEX. I'm not sure what we
> should do about it, or if we even want to do something about it. If we
> want REINDEX to fail in this case, we should be sure to check
> permissions on everything up-front to avoid doing a lot of work. The
> only other option I can think of is to REINDEX only those indexes
> declared on the partitioned table (not the individual partitions),
> which seems consistent but might be confusing to users.

At the moment, I think I'm inclined to call this "existing behavior" since
we didn't check privileges for each partition in this case even before
MAINTAIN was introduced.  IIUC we still process the individual partitions
in v15 regardless of whether the calling user owns the partition.

However, I do agree that it feels inconsistent.  Besides the options you
proposed, we might also consider making REINDEX work a bit more like VACUUM
and ANALYZE and emit a WARNING for any relations that the user is not
permitted to process.  But this probably deserves its own thread, and it
might even need to wait until v17.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Michael Paquier <michael@paquier.xyz> — 2023-06-20T22:53:52Z

On Tue, Jun 20, 2023 at 11:43:05AM -0700, Jeff Davis wrote:
> The only behavior I'm worried about is REINDEX. I'm not sure what we
> should do about it, or if we even want to do something about it. If we
> want REINDEX to fail in this case, we should be sure to check
> permissions on everything up-front to avoid doing a lot of work.

Yes, that feels a bit inconsistent to only check the partitioned table
in RangeVarCallbackForReindexIndex() and let all the partitions
process as a user may not have the permissions to work on the
partitions themselves.  We'd need something close to
expand_vacuum_rel() for this work.  I am not sure that this level of
change is required, TBH, still it could be discussed for v17~.

> The
> only other option I can think of is to REINDEX only those indexes
> declared on the partitioned table (not the individual partitions),
> which seems consistent but might be confusing to users.

I am not sure to understand this last sentence.  REINDEX on a
partitioned table builds a list of the indexes to work on in the first
transaction processing the command in ReindexPartitions(), and there
is no need to process partitioned indexes as these have no storage, so
your suggestion is a no-op?
--
Michael

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Michael Paquier <michael@paquier.xyz> — 2023-06-21T01:21:04Z

On Tue, Jun 20, 2023 at 03:52:57PM -0700, Nathan Bossart wrote:
> However, I do agree that it feels inconsistent.  Besides the options you
> proposed, we might also consider making REINDEX work a bit more like VACUUM
> and ANALYZE and emit a WARNING for any relations that the user is not
> permitted to process.  But this probably deserves its own thread, and it
> might even need to wait until v17.

Looking at 0001..

-step s2_auth           { SET ROLE regress_cluster_part; }
+step s2_auth           { SET ROLE regress_cluster_part; SET client_min_messages = ERROR; }

Is this change necessary because the ordering of the WARNING messages
generated for denied permissions is not guaranteed?

From the generated vacuum.out:
-- Only one partition owned by other user.
ALTER TABLE vacowned_parted OWNER TO CURRENT_USER;
SET ROLE regress_vacuum;
VACUUM vacowned_parted;
WARNING:  permission denied to vacuum "vacowned_parted", skipping it
WARNING:  permission denied to vacuum "vacowned_part2", skipping it

This is interesting.  In this case, regress_vacuum owns only one
partition, but we would be able to vacuum it even when querying
vacowned_parted.  Seeing from [1], this is intentional as per the
argument that VACUUM/ANALYZE can take multiple relations.  Am I
getting that right?  That's different from CLUSTER or REINDEX, where
not owning the partitioned table fails immediately.

I think that there is a testing gap with the coverage of CLUSTER.
"Ownership of partitions is checked" is a test that looks for the case
where regress_ptnowner owns the partitioned table and one of its
partitions, checking that the leaf not owned is skipped, but we don't
have a test where we attempt a CLUSTER on the partitioned table with
regress_ptnowner *not* owning the partitioned table, only one or more
of its partitions owned by regress_ptnowner.  In this case, the
command would fail.

-   privilege on the catalog.  If a role has permission to
-   <command>REINDEX</command> a partitioned table, it is also permitted to
-   <command>REINDEX</command> each of its partitions, regardless of whether the
-   role has the aforementioned privileges on the partition.  Of course,
-   superusers can always reindex anything.
+   privilege on the catalog.  Of course, superusers can always reindex anything.

With 0001 applied, if a user is marked as an owner of a partitioned
table, all the partitions are reindexed even if this user does not own
a portion of them, making this change incorrect while the former is
more correct?

[1]: https://www.postgresql.org/message-id/20221216011926.GA771496@nathanxps13
--
Michael

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2023-06-21T04:15:18Z

On Wed, Jun 21, 2023 at 10:21:04AM +0900, Michael Paquier wrote:
> Looking at 0001..

Thanks for taking a look.

> -step s2_auth           { SET ROLE regress_cluster_part; }
> +step s2_auth           { SET ROLE regress_cluster_part; SET client_min_messages = ERROR; }
> 
> Is this change necessary because the ordering of the WARNING messages
> generated for denied permissions is not guaranteed?

Yes.

> From the generated vacuum.out:
> -- Only one partition owned by other user.
> ALTER TABLE vacowned_parted OWNER TO CURRENT_USER;
> SET ROLE regress_vacuum;
> VACUUM vacowned_parted;
> WARNING:  permission denied to vacuum "vacowned_parted", skipping it
> WARNING:  permission denied to vacuum "vacowned_part2", skipping it
> 
> This is interesting.  In this case, regress_vacuum owns only one
> partition, but we would be able to vacuum it even when querying
> vacowned_parted.  Seeing from [1], this is intentional as per the
> argument that VACUUM/ANALYZE can take multiple relations.  Am I
> getting that right?  That's different from CLUSTER or REINDEX, where
> not owning the partitioned table fails immediately.

Yes.

> I think that there is a testing gap with the coverage of CLUSTER.
> "Ownership of partitions is checked" is a test that looks for the case
> where regress_ptnowner owns the partitioned table and one of its
> partitions, checking that the leaf not owned is skipped, but we don't
> have a test where we attempt a CLUSTER on the partitioned table with
> regress_ptnowner *not* owning the partitioned table, only one or more
> of its partitions owned by regress_ptnowner.  In this case, the
> command would fail.

We could add something for this, but it'd really just exercise the checks
in RangeVarCallbackMaintainsTable(), which already has a decent amount of
coverage.

> -   privilege on the catalog.  If a role has permission to
> -   <command>REINDEX</command> a partitioned table, it is also permitted to
> -   <command>REINDEX</command> each of its partitions, regardless of whether the
> -   role has the aforementioned privileges on the partition.  Of course,
> -   superusers can always reindex anything.
> +   privilege on the catalog.  Of course, superusers can always reindex anything.
> 
> With 0001 applied, if a user is marked as an owner of a partitioned
> table, all the partitions are reindexed even if this user does not own
> a portion of them, making this change incorrect while the former is
> more correct?

The former wording would be true from the perspective that REINDEX on a
partitioned table will flow down to its partitions and skip privilege
checks on them, but it's incomplete because REINDEX on the individual
partitions might still fail due to privileges (even if the user has
privileges to REINDEX the partitioned table).  After both patches are
applied, the privilege documentation is distilled down to

	Reindexing a single index or table requires having the MAINTAIN
	privilege on the table.

plus some assorted notes about REINDEX DATABASE/SCHEMA/SYSTEM.  I think the
proposed wording is accurate, but I can see the argument that it leaves
some ambiguity for the partitioned table case.  Perhaps we should add
something like

	Note that while REINDEX on a partitioned index or table requires
	MAINTAIN on the partitioned table, such commands skip the privilege
	checks when processing the individual partitions.

Thoughts?  I'm trying to keep the privilege documentation for maintenance
commands as simple as possible, so I'm hoping to avoid adding too much text
dedicated to these special cases.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Jeff Davis <pgsql@j-davis.com> — 2023-06-21T16:26:09Z

On Wed, 2023-06-21 at 07:53 +0900, Michael Paquier wrote:
> I am not sure to understand this last sentence.  REINDEX on a
> partitioned table builds a list of the indexes to work on in the
> first
> transaction processing the command in ReindexPartitions(), and there
> is no need to process partitioned indexes as these have no storage,
> so
> your suggestion is a no-op?

What I meant is that if you do:

  CREATE TABLE p(i INT, j INT) PARTITION BY RANGE (i);
  CREATE TABLE p0 PARTITION OF p FOR VALUES FROM (00) TO (10);
  CREATE TABLE p1 PARTITION OF p FOR VALUES FROM (10) TO (20);
  CREATE INDEX p_idx ON p (i);
  CREATE INDEX special_idx ON p0 (j);
  GRANT MAINTAIN ON p TO foo;
  \c - foo
  REINDEX TABLE p;

That would reindex p0_i_idx and p1_i_idx, but skip special_idx. That
might be too confusing, but feels a bit more consistent permissions-
wise.

Regards,
	Jeff Davis

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Jeff Davis <pgsql@j-davis.com> — 2023-06-21T16:29:54Z

On Tue, 2023-06-20 at 15:52 -0700, Nathan Bossart wrote:
> At the moment, I think I'm inclined to call this "existing behavior"
> since
> we didn't check privileges for each partition in this case even
> before
> MAINTAIN was introduced.  IIUC we still process the individual
> partitions
> in v15 regardless of whether the calling user owns the partition.

That's fine with me. I just wanted to bring it up in case someone else
thought it was a problem.

> However, I do agree that it feels inconsistent.  Besides the options
> you
> proposed, we might also consider making REINDEX work a bit more like
> VACUUM
> and ANALYZE and emit a WARNING for any relations that the user is not
> permitted to process.  But this probably deserves its own thread, and
> it
> might even need to wait until v17.

Yes, we can revisit for 17.

Regards,
	Jeff Davis

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2023-06-21T17:16:24Z

On Tue, Jun 20, 2023 at 09:15:18PM -0700, Nathan Bossart wrote:
> Perhaps we should add something like
> 
> 	Note that while REINDEX on a partitioned index or table requires
> 	MAINTAIN on the partitioned table, such commands skip the privilege
> 	checks when processing the individual partitions.
> 
> Thoughts?  I'm trying to keep the privilege documentation for maintenance
> commands as simple as possible, so I'm hoping to avoid adding too much text
> dedicated to these special cases.

Here is a new patch set that includes this new sentence.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Michael Paquier <michael@paquier.xyz> — 2023-06-22T00:50:54Z

On Wed, Jun 21, 2023 at 09:26:09AM -0700, Jeff Davis wrote:
> What I meant is that if you do:
> 
>   CREATE TABLE p(i INT, j INT) PARTITION BY RANGE (i);
>   CREATE TABLE p0 PARTITION OF p FOR VALUES FROM (00) TO (10);
>   CREATE TABLE p1 PARTITION OF p FOR VALUES FROM (10) TO (20);
>   CREATE INDEX p_idx ON p (i);
>   CREATE INDEX special_idx ON p0 (j);
>   GRANT MAINTAIN ON p TO foo;
>   \c - foo
>   REINDEX TABLE p;
> 
> That would reindex p0_i_idx and p1_i_idx, but skip special_idx. That
> might be too confusing, but feels a bit more consistent permissions-
> wise.

FWIW, the current behavior to reindex special_idx in this case feels
more natural to me, as the user requests a REINDEX at table-level, not
at index-level.  This would mean to me that all the indexes of all the
partitions should be rebuilt on, not just the partitioned indexes that
are defined in the partitioned table requested for rebuild.
--
Michael

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Michael Paquier <michael@paquier.xyz> — 2023-06-22T01:46:41Z

On Wed, Jun 21, 2023 at 10:16:24AM -0700, Nathan Bossart wrote:
>> I think that there is a testing gap with the coverage of CLUSTER.
>> "Ownership of partitions is checked" is a test that looks for the case
>> where regress_ptnowner owns the partitioned table and one of its
>> partitions, checking that the leaf not owned is skipped, but we don't
>> have a test where we attempt a CLUSTER on the partitioned table with
>> regress_ptnowner *not* owning the partitioned table, only one or more
>> of its partitions owned by regress_ptnowner.  In this case, the
>> command would fail.
>  
> We could add something for this, but it'd really just exercise the checks
> in RangeVarCallbackMaintainsTable(), which already has a decent amount of
> coverage.

It seems to me that this has some value for the CLUSTER path, so I
would add a small thing for it.

> On Tue, Jun 20, 2023 at 09:15:18PM -0700, Nathan Bossart wrote:
>> Perhaps we should add something like
>> 
>> 	Note that while REINDEX on a partitioned index or table requires
>> 	MAINTAIN on the partitioned table, such commands skip the privilege
>> 	checks when processing the individual partitions.
>> 
>> Thoughts?  I'm trying to keep the privilege documentation for maintenance
>> commands as simple as possible, so I'm hoping to avoid adding too much text
>> dedicated to these special cases.
> 
> Here is a new patch set that includes this new sentence.

-       aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_INDEX,
-                      relation->relname);
Interesting that the previous code assumed ACLCHECK_NOT_OWNER all the
time in the reindex RangeVar callback.

-       /*
-        * We already checked that the user has privileges to CLUSTER the
-        * partitioned table when we locked it earlier, so there's no need to
-        * check the privileges again here.
-        */
+       if (!cluster_is_permitted_for_relation(relid, GetUserId()))
+           continue;
I would add a comment here that this ACL recheck for the leaves is an
important thing to keep around as it impacts the case where the leaves
have a different owner than the parent, and the owner of the parent
clusters it.  The only place in the tests where this has an influence
is the isolation test cluster-conflict-partition.

The documentation changes seem in line with the code changes,
particularly for VACUUM and REINDEX where we have some special
handling for shared catalogs with ownership.
--
Michael

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2023-06-22T03:06:06Z

On Thu, Jun 22, 2023 at 10:46:41AM +0900, Michael Paquier wrote:
> On Wed, Jun 21, 2023 at 10:16:24AM -0700, Nathan Bossart wrote:
>>> I think that there is a testing gap with the coverage of CLUSTER.
>>> "Ownership of partitions is checked" is a test that looks for the case
>>> where regress_ptnowner owns the partitioned table and one of its
>>> partitions, checking that the leaf not owned is skipped, but we don't
>>> have a test where we attempt a CLUSTER on the partitioned table with
>>> regress_ptnowner *not* owning the partitioned table, only one or more
>>> of its partitions owned by regress_ptnowner.  In this case, the
>>> command would fail.
>>  
>> We could add something for this, but it'd really just exercise the checks
>> in RangeVarCallbackMaintainsTable(), which already has a decent amount of
>> coverage.
> 
> It seems to me that this has some value for the CLUSTER path, so I
> would add a small thing for it.

Done.

> -       /*
> -        * We already checked that the user has privileges to CLUSTER the
> -        * partitioned table when we locked it earlier, so there's no need to
> -        * check the privileges again here.
> -        */
> +       if (!cluster_is_permitted_for_relation(relid, GetUserId()))
> +           continue;
> I would add a comment here that this ACL recheck for the leaves is an
> important thing to keep around as it impacts the case where the leaves
> have a different owner than the parent, and the owner of the parent
> clusters it.  The only place in the tests where this has an influence
> is the isolation test cluster-conflict-partition.

Done.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Michael Paquier <michael@paquier.xyz> — 2023-06-22T07:11:08Z

On Wed, Jun 21, 2023 at 08:06:06PM -0700, Nathan Bossart wrote:
> On Thu, Jun 22, 2023 at 10:46:41AM +0900, Michael Paquier wrote:
>> -       /*
>> -        * We already checked that the user has privileges to CLUSTER the
>> -        * partitioned table when we locked it earlier, so there's no need to
>> -        * check the privileges again here.
>> -        */
>> +       if (!cluster_is_permitted_for_relation(relid, GetUserId()))
>> +           continue;
>> I would add a comment here that this ACL recheck for the leaves is an
>> important thing to keep around as it impacts the case where the leaves
>> have a different owner than the parent, and the owner of the parent
>> clusters it.  The only place in the tests where this has an influence
>> is the isolation test cluster-conflict-partition.
> 
> Done.

+       /*
+        * It's possible that the user does not have privileges to CLUSTER the
+        * leaf partition despite having such privileges on the partitioned
+        * table.  We skip any partitions which the user is not permitted to
+        * CLUSTER.
+        */

Sounds good to me.  Thanks.
--
Michael

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2023-06-22T15:43:01Z

On Thu, Jun 22, 2023 at 04:11:08PM +0900, Michael Paquier wrote:
> Sounds good to me.  Thanks.

I plan to commit these patches later today.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX

Nathan Bossart <nathandbossart@gmail.com> — 2023-06-22T23:01:00Z

On Thu, Jun 22, 2023 at 08:43:01AM -0700, Nathan Bossart wrote:
> I plan to commit these patches later today.

Committed.  I've also marked the related open item for v16 as resolved.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com