Thread

Commits

Clarify documentation for CLUSTER on partitioned tables.
- 8b5f36bb6c16 16.0 landed
Fix MAINTAIN privileges for toast tables and partitions.
- ff9618e82a46 16.0 cited
Have CLUSTER ignore partitions not owned by caller
- 3f19e176ae0f 15.0 cited

fix and document CLUSTER privileges

Nathan Bossart <nathandbossart@gmail.com> — 2022-12-07T22:39:24Z

Hi hackers,

While looking into other opportunities for per-table permissions, I noticed
a weird discrepancy in CLUSTER.  When evaluating whether the current user
has permission to CLUSTER a table, we ordinarily just check for ownership.
However, the database owner is also allowed to CLUSTER all partitions that
are not shared.  This was added in 3f19e17, and I didn't see any discussion
about it in the corresponding thread [0].

My first instinct is that we should just remove the database ownership
check, which is what I've done in the attached patch.  I don't see any
strong reason to complicate matters with special
database-owner-but-not-shared checks like other commands (e.g., VACUUM).
But perhaps we should do so just for consistency's sake.  Thoughts?

It was also noted elsewhere [1] that the privilege requirements for CLUSTER
are not documented.  The attached patch adds such documentation.

[0] https://postgr.es/m/20220411140609.GF26620%40telsasoft.com
[1] https://postgr.es/m/661148f4-c7f1-dec1-2bc8-29f3bd58e242%40postgrespro.ru

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

Re: fix and document CLUSTER privileges

Justin Pryzby <pryzby@telsasoft.com> — 2022-12-08T02:25:59Z

On Wed, Dec 07, 2022 at 02:39:24PM -0800, Nathan Bossart wrote:
> Hi hackers,
> 
> While looking into other opportunities for per-table permissions, I noticed
> a weird discrepancy in CLUSTER.  When evaluating whether the current user
> has permission to CLUSTER a table, we ordinarily just check for ownership.
> However, the database owner is also allowed to CLUSTER all partitions that
> are not shared.  This was added in 3f19e17, and I didn't see any discussion
> about it in the corresponding thread [0].
> 
> My first instinct is that we should just remove the database ownership
> check, which is what I've done in the attached patch.  I don't see any
> strong reason to complicate matters with special
> database-owner-but-not-shared checks like other commands (e.g., VACUUM).
> But perhaps we should do so just for consistency's sake.  Thoughts?

Your patch makes it inconsistent with vacuum full, which is strange
because vacuum full calls cluster.

postgres=> VACUUM FULL t;
VACUUM
postgres=> CLUSTER t;
ERROR:  must be owner of table t

BTW, it'd be helpful to copy the relevant parties on this kind of
message, especially if there's a new thread dedicated just to this.

-- 
Justin

Re: fix and document CLUSTER privileges

Nathan Bossart <nathandbossart@gmail.com> — 2022-12-08T04:13:13Z

On Wed, Dec 07, 2022 at 08:25:59PM -0600, Justin Pryzby wrote:
> Your patch makes it inconsistent with vacuum full, which is strange
> because vacuum full calls cluster.
> 
> postgres=> VACUUM FULL t;
> VACUUM
> postgres=> CLUSTER t;
> ERROR:  must be owner of table t

This is the existing behavior on HEAD.  I think it has been this way for a
while.  Granted, that doesn't mean it's ideal, but AFAICT it's intentional.

Looking closer, the database ownership check in
get_tables_to_cluster_partitioned() appears to have no meaningful effect.
In this code path, cluster_rel() will always be called with CLUOPT_RECHECK,
and that function only checks for table ownership.  Anything gathered in
get_tables_to_cluster_partitioned() that the user doesn't own will be
skipped.  So, I don't think my patch changes the behavior in any meaningful
way, but I still think it's worthwhile to make the checks consistent.

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

Re: fix and document CLUSTER privileges

Pavel Luzanov <p.luzanov@postgrespro.ru> — 2022-12-08T11:15:58Z

On 08.12.2022 01:39, Nathan Bossart wrote:
> It was also noted elsewhere [1] that the privilege requirements for CLUSTER
> are not documented.  The attached patch adds such documentation.
> [1] https://postgr.es/m/661148f4-c7f1-dec1-2bc8-29f3bd58e242%40postgrespro.ru

Thanks for the patch. It correctly states the existing behavior.

But perhaps we should wait for the decision in discussion [1] (link above),
since this decision may affect the documentation on the CLUSTER command.

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

Re: fix and document CLUSTER privileges

Andrew Dunstan <andrew@dunslane.net> — 2022-12-08T12:20:28Z

On 2022-12-07 We 23:13, Nathan Bossart wrote:
> On Wed, Dec 07, 2022 at 08:25:59PM -0600, Justin Pryzby wrote:
>> Your patch makes it inconsistent with vacuum full, which is strange
>> because vacuum full calls cluster.
>>
>> postgres=> VACUUM FULL t;
>> VACUUM
>> postgres=> CLUSTER t;
>> ERROR:  must be owner of table t
> This is the existing behavior on HEAD.  I think it has been this way for a
> while.  Granted, that doesn't mean it's ideal, but AFAICT it's intentional.
>
> Looking closer, the database ownership check in
> get_tables_to_cluster_partitioned() appears to have no meaningful effect.
> In this code path, cluster_rel() will always be called with CLUOPT_RECHECK,
> and that function only checks for table ownership.  Anything gathered in
> get_tables_to_cluster_partitioned() that the user doesn't own will be
> skipped.  So, I don't think my patch changes the behavior in any meaningful
> way, but I still think it's worthwhile to make the checks consistent.



We should probably talk about what the privileges should be, though. I
think there's a case to be made that CLUSTER should be governed by the
VACUUM privileges, given how VACUUM FULL is now implemented.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: fix and document CLUSTER privileges

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

On Thu, Dec 08, 2022 at 07:20:28AM -0500, Andrew Dunstan wrote:
> We should probably talk about what the privileges should be, though. I
> think there's a case to be made that CLUSTER should be governed by the
> VACUUM privileges, given how VACUUM FULL is now implemented.

Currently, CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX (minus REINDEX
SCHEMA|DATABASE|SYSTEM) require ownership of the relation or superuser.  In
fact, all three use the same RangeVarCallbackOwnsTable() callback function.
My current thinking is that this is good enough.  I don't sense any strong
demand for allowing database owners to run these commands on all non-shared
relations, and there's ongoing work to break out the privileges to GRANT
and predefined roles.  However, I don't have a strong opinion about this.

If we do want to change the permissions model for CLUSTER, it might make
sense to change all three of the aforementioned commands to look more like
VACUUM/ANALYZE.

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

Re: fix and document CLUSTER privileges

Robert Haas <robertmhaas@gmail.com> — 2022-12-08T21:08:40Z

On Thu, Dec 8, 2022 at 1:13 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> On Thu, Dec 08, 2022 at 07:20:28AM -0500, Andrew Dunstan wrote:
> > We should probably talk about what the privileges should be, though. I
> > think there's a case to be made that CLUSTER should be governed by the
> > VACUUM privileges, given how VACUUM FULL is now implemented.
>
> Currently, CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX (minus REINDEX
> SCHEMA|DATABASE|SYSTEM) require ownership of the relation or superuser.  In
> fact, all three use the same RangeVarCallbackOwnsTable() callback function.
> My current thinking is that this is good enough.  I don't sense any strong
> demand for allowing database owners to run these commands on all non-shared
> relations, and there's ongoing work to break out the privileges to GRANT
> and predefined roles.

+1.

I don't see why being the database owner should give you the right to
run a random subset of commands on any table in the database. Tables
have their own system for access privileges; we should use that, or
extend it as required.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: fix and document CLUSTER privileges

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

On Thu, Dec 08, 2022 at 04:08:40PM -0500, Robert Haas wrote:
> On Thu, Dec 8, 2022 at 1:13 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> Currently, CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX (minus REINDEX
>> SCHEMA|DATABASE|SYSTEM) require ownership of the relation or superuser.  In
>> fact, all three use the same RangeVarCallbackOwnsTable() callback function.
>> My current thinking is that this is good enough.  I don't sense any strong
>> demand for allowing database owners to run these commands on all non-shared
>> relations, and there's ongoing work to break out the privileges to GRANT
>> and predefined roles.
> 
> +1.
> 
> I don't see why being the database owner should give you the right to
> run a random subset of commands on any table in the database. Tables
> have their own system for access privileges; we should use that, or
> extend it as required.

Here is a rebased version of the patch.

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

Re: fix and document CLUSTER privileges

Nathan Bossart <nathandbossart@gmail.com> — 2022-12-16T04:57:00Z

Here is a new version of the patch.  I've moved the privilege checks to a
new function, and I added a note in the docs about clustering partitioned
tables in a transaction block (it's not allowed).

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

Re: fix and document CLUSTER privileges

Gilles Darold <gilles@migops.com> — 2023-01-04T13:25:13Z

Le 16/12/2022 à 05:57, Nathan Bossart a écrit :
> Here is a new version of the patch.  I've moved the privilege checks to a
> new function, and I added a note in the docs about clustering partitioned
> tables in a transaction block (it's not allowed).
>

Getting into review of this patch I wonder why the CLUSTER command do 
not react as VACUUM FULL command when there is insuffisant privileges. 
For example with a partitioned table (ptnowner) and two partitions 
(ptnowner1 and ptnowner2) with the second partition owned by another 
user, let' say usr2. We have the following report when executing vacuum 
as usr2:

testdb=> VACUUM FULL ptnowner;
WARNING:  permission denied to vacuum "ptnowner", skipping it
WARNING:  permission denied to vacuum "ptnowner1", skipping it
VACUUM

Here only ptnowner2 have been vacuumed which is correct and expected.

For the cluster command:

testdb=> CLUSTER;
CLUSTER


I would have expected something like:

testdb=> CLUSTER;
WARNING:  permission denied to cluster "ptnowner1", skipping it
CLUSTER

I mean that the silent behavior is not very helpful.


This is the current behavior of the CLUSTER command and current patch 
adds a sentence about the silent behavior in the documentation. This is 
good but I just want to ask if we could want to fix this behavior too or 
just keep things like that with the lack of noise.


Best regards,

-- 
Gilles Darold

Re: fix and document CLUSTER privileges

Nathan Bossart <nathandbossart@gmail.com> — 2023-01-04T18:18:56Z

On Wed, Jan 04, 2023 at 02:25:13PM +0100, Gilles Darold wrote:
> This is the current behavior of the CLUSTER command and current patch adds a
> sentence about the silent behavior in the documentation. This is good but I
> just want to ask if we could want to fix this behavior too or just keep
> things like that with the lack of noise.

I've proposed something like what you are describing in another thread [0].
I intended to simply fix and document the current behavior in this thread
and to take up any new changes in the other one.

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

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

Re: fix and document CLUSTER privileges

Gilles Darold <gilles@migops.com> — 2023-01-04T22:27:05Z

Le 04/01/2023 à 19:18, Nathan Bossart a écrit :
> On Wed, Jan 04, 2023 at 02:25:13PM +0100, Gilles Darold wrote:
>> This is the current behavior of the CLUSTER command and current patch adds a
>> sentence about the silent behavior in the documentation. This is good but I
>> just want to ask if we could want to fix this behavior too or just keep
>> things like that with the lack of noise.
> I've proposed something like what you are describing in another thread [0].
> I intended to simply fix and document the current behavior in this thread
> and to take up any new changes in the other one.
>
> [0] https://commitfest.postgresql.org/41/4070/


Got it, this is patch add_cluster_skip_messages.patch . IMHO this patch 
should be part of this commitfest as it is directly based on this one. 
You could create a second patch here that adds the warning message so 
that committers can decide here if it should be applied.


-- 
Gilles Darold

Re: fix and document CLUSTER privileges

Nathan Bossart <nathandbossart@gmail.com> — 2023-01-05T05:12:35Z

On Wed, Jan 04, 2023 at 11:27:05PM +0100, Gilles Darold wrote:
> Got it, this is patch add_cluster_skip_messages.patch . IMHO this patch
> should be part of this commitfest as it is directly based on this one. You
> could create a second patch here that adds the warning message so that
> committers can decide here if it should be applied.

That's fine with me.  I added the warning messages in v4.

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

Re: fix and document CLUSTER privileges

Gilles Darold <gilles@migops.com> — 2023-01-05T13:38:58Z

Le 05/01/2023 à 06:12, Nathan Bossart a écrit :
> On Wed, Jan 04, 2023 at 11:27:05PM +0100, Gilles Darold wrote:
>> Got it, this is patch add_cluster_skip_messages.patch . IMHO this patch
>> should be part of this commitfest as it is directly based on this one. You
>> could create a second patch here that adds the warning message so that
>> committers can decide here if it should be applied.
> That's fine with me.  I added the warning messages in v4.

This is a bit confusing, this commitfest entry patch is also included in 
an other commitfest entry [1] into patch 
v3-0001-fix-maintain-privs.patch with some additional conditions.

Committers should be aware that this commitfest entry must be withdrawn 
if [1] is committed first.  There is no status or dependency field that 
I can use, I could move this one to "Ready for Committer" status but 
this is not exact until [1] has been committed or withdrawn.

[1] https://commitfest.postgresql.org/41/4070/

-- 
Gilles Darold

Re: fix and document CLUSTER privileges

Nathan Bossart <nathandbossart@gmail.com> — 2023-01-05T17:11:42Z

On Thu, Jan 05, 2023 at 02:38:58PM +0100, Gilles Darold wrote:
> This is a bit confusing, this commitfest entry patch is also included in an
> other commitfest entry [1] into patch v3-0001-fix-maintain-privs.patch with
> some additional conditions.
> 
> Committers should be aware that this commitfest entry must be withdrawn if
> [1] is committed first.  There is no status or dependency field that I can
> use, I could move this one to "Ready for Committer" status but this is not
> exact until [1] has been committed or withdrawn.

I will either rebase the other patch or discard this one as needed.  I'm
not positive that we'll proceed with the proposed approach for the other
one, but the patch tracked here should still be worthwhile regardless.

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

Re: fix and document CLUSTER privileges

Nathan Bossart <nathandbossart@gmail.com> — 2023-01-06T00:26:46Z

Apparently I forgot to run all the tests before posting v4.  Here is a new
version of the patch that should pass all tests.

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

Re: fix and document CLUSTER privileges

Gilles Darold <gilles@migops.com> — 2023-01-11T13:22:26Z

Le 06/01/2023 à 01:26, Nathan Bossart a écrit :
> Apparently I forgot to run all the tests before posting v4.  Here is a new
> version of the patch that should pass all tests.

Review status:


The patch applies and compiles without issues, make check and 
checkinstall tests are running without error.

It aim to limit the permission check to run the CLUSTER command on a 
partition to ownership and the MAINTAIN privilege. Which it actually does.

In commit 3f19e17, to have CLUSTER ignore partitions not owned by 
caller, there was still a useless check of database ownership or shared 
relation in get_tables_to_cluster_partitioned().


Documentation have been updated to explain the conditions of a 
successful execution of the CLUSTER command.


Additionally this patch also adds a warning when a partition is skipped 
due to lack of permission just like VACUUM is doing:

     WARNING:  permission denied to vacuum "ptnowner2", skipping it

with CLUSTER now we have the same message:

     WARNING:  permission denied to cluster "ptnowner2", skipping it

Previous behavior was to skip the partition silently.


Tests on the CLUSTER command have been modified to skip warning messages 
except partially in src/test/regress/sql/cluster.sql to validate the 
presence of the warning.


I'm moving this commitfest entry to Ready for Committers.


Regards,

-- 
Gilles Darold

Re: fix and document CLUSTER privileges

Nathan Bossart <nathandbossart@gmail.com> — 2023-01-11T17:54:17Z

On Wed, Jan 11, 2023 at 02:22:26PM +0100, Gilles Darold wrote:
> I'm moving this commitfest entry to Ready for Committers.

Thank you for reviewing.

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

Re: fix and document CLUSTER privileges

Gilles Darold <gilles@migops.com> — 2023-01-14T09:40:40Z

Le 11/01/2023 à 18:54, Nathan Bossart a écrit :
> On Wed, Jan 11, 2023 at 02:22:26PM +0100, Gilles Darold wrote:
>> I'm moving this commitfest entry to Ready for Committers.
> Thank you for reviewing.
>
I have changed the status to "Returned with feedback" as per commit 
ff9618e8 this patch might not be applied anymore if I have well understood.


Nathan, please confirm and fix the status of this commit fest entry.


Best regards,

-- 
Gilles Darold

Re: fix and document CLUSTER privileges

Nathan Bossart <nathandbossart@gmail.com> — 2023-01-14T22:40:00Z

On Sat, Jan 14, 2023 at 10:40:40AM +0100, Gilles Darold wrote:
> Nathan, please confirm and fix the status of this commit fest entry.

Yes, thank you for taking care of this.  I believe the only changes in this
patch that didn't make it into ff9618e are the following documentation
adjustments.  I've added Jeff to get his thoughts.

diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index b9f2acb1de..29f0f1fd90 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -67,7 +67,8 @@ CLUSTER [VERBOSE]
   </para>
 
   <para>
-   <command>CLUSTER</command> without any parameter reclusters all the
+   <command>CLUSTER</command> without a
+   <replaceable class="parameter">table_name</replaceable> reclusters all the
    previously-clustered tables in the current database that the calling user
    has privileges for.  This form of <command>CLUSTER</command> cannot be
    executed inside a transaction block.
@@ -211,7 +212,8 @@ CLUSTER [VERBOSE]
    <para>
     Clustering a partitioned table clusters each of its partitions using the
     partition of the specified partitioned index.  When clustering a partitioned
-    table, the index may not be omitted.
+    table, the index may not be omitted.  <command>CLUSTER</command> on a
+    partitioned table cannot be executed inside a transaction block.
    </para>
 
  </refsect1>

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

Re: fix and document CLUSTER privileges

Jeff Davis <pgsql@j-davis.com> — 2023-01-26T04:27:57Z

On Sat, 2023-01-14 at 14:40 -0800, Nathan Bossart wrote:
> On Sat, Jan 14, 2023 at 10:40:40AM +0100, Gilles Darold wrote:
> > Nathan, please confirm and fix the status of this commit fest
> > entry.
> 
> Yes, thank you for taking care of this.  I believe the only changes
> in this
> patch that didn't make it into ff9618e are the following
> documentation
> adjustments.  I've added Jeff to get his thoughts.

Committed these extra clarifications. Thank you.


-- 
Jeff Davis
PostgreSQL Contributor Team - AWS

Re: fix and document CLUSTER privileges

Nathan Bossart <nathandbossart@gmail.com> — 2023-01-26T18:01:16Z

On Wed, Jan 25, 2023 at 08:27:57PM -0800, Jeff Davis wrote:
> Committed these extra clarifications. Thank you.

Thanks!

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