Re: fix and document CLUSTER privileges
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
Re: fix and document CLUSTER privileges
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
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] - CLUSTER without any parameter reclusters all the + CLUSTER without a + table_name reclusters all the previously-clustered tables in the current database that the calling user has privileges for. This form of CLUSTER cannot be executed inside a transaction block. @@ -211,7 +212,8 @@ CLUSTER [VERBOSE] 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. CLUSTER on a +partitioned table cannot be executed inside a transaction block. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: fix and document CLUSTER privileges
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
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
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
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
diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index 145101e6a5..749b410e16 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -67,7 +67,8 @@ CLUSTER [VERBOSE]
- CLUSTER without any parameter reclusters all the
+ CLUSTER without a
+ table_name reclusters all the
previously-clustered tables in the current database that the calling user
owns or has the MAINTAIN privilege for, or all such tables
if called by a superuser or a role with privileges of the
@@ -134,6 +135,15 @@ CLUSTER [VERBOSE]
Notes
+
+To cluster a table, one must have the MAINTAIN privilege
+on the table or be the table's owner, a superuser, or a role with
+privileges of the
+pg_maintain
+role. Database-wide clusters and clusters on partitioned tables will skip
+over any tables that the calling user does not have permission to cluster.
+
+
In cases where you are accessing single rows randomly
within a table, the actual order of the data in the
@@ -202,7 +212,8 @@ CLUSTER [VERBOSE]
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. CLUSTER on a
+partitioned table cannot be executed inside a transaction block.
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index f11691aff7..55b699ef05 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -80,6 +80,7 @@ static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
static List *get_tables_to_cluster(MemoryContext cluster_context);
static List *get_tables_to_cluster_partitioned(MemoryContext cluster_context,
Oid indexOid);
+static bool cluster_is_permitted_for_relation(Oid relid, Oid userid);
/*---
@@ -366,8 +367,7 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
if (recheck)
{
/* Check that the user still has privileges for the relation */
- if (!object_ownercheck(RelationRelationId, tableOid, save_userid) &&
- pg_class_aclcheck(tableOid, save_userid, ACL_MAINTAIN) != ACLCHECK_OK)
+ if (!cluster_is_permitted_for_relation(tableOid, save_userid))
{
relation_close(OldHeap, AccessExclusiveLock);
goto out;
@@ -1645,8 +1645,7 @@ get_tables_to_cluster(MemoryContext cluster_context)
index = (Form_pg_index) GETSTRUCT(indexTuple);
- if (!object_ownercheck(RelationRelationId, index->indrelid, GetUserId()) &&
- pg_class_aclcheck(index->indrelid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK)
+ if (!cluster_is_permitted_for_relation(index->indrelid, GetUserId()))
continue;
/* Use a permanent memory context for the result list */
@@ -1695,10 +1694,7 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
continue;
/* Silently skip partitions which the user has no access to. */
- if (!object_ownercheck(RelationRelationId, relid, GetUserId()) &&
- pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK &&
- (!object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) ||
- IsSharedRelation(relid)))
+ if (!cluster_is_permitted_for_relation(relid, GetUserId()))
continue;
/* Use a permanent memory context for the result list */
@@ -1714,3 +1710,16 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
return rtcs;
}
+
+static bool
+cluster_is_permitted_for_relation(Oid relid, Oid userid)
+{
+ if (object_ownercheck(RelationRelationId, relid, userid) ||
+ pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK)
+ return true;
+
+ ereport(WARNING,
+ (errmsg("permission denied to cluster \"%s\", skipping it",
+ get_rel_name(relid;
+ return false;
+}
diff --git a/src/test/isolation/expected/cluster-conflict-partition.out b/src/test/isolation/expected/cluster-conflict-partition.out
index 7acb675c97..9e31fc06d9 100644
--- a/src/test/isolation/expected/cluster-conflict-partition.out
+++ b/src/test/isolation/expected/cluster-conflict-partition.out
@@ -3,7 +3,7 @@ Parsed test spec with 2 sessions
starting permutation: s1_begin s1_lock_parent s2_auth s2_cluster s1_commit s2_reset
step s1_begin: BEGIN;
step s1_lock_parent: LOCK cluster_part_tab IN SHARE UPDATE EXCLUSIVE MODE;
-step s2_auth: SET ROLE regress_cluster_part;
+step s2_auth: SET ROLE regress_cluster_part; SET client_min_messages = 'ERROR';
step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind;
step s1_commit: COMMIT;
step s2_cluster: <... completed>
@@ -11,7 +11,7 @@ step s2_reset:
Re: fix and document CLUSTER privileges
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
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
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
diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index 145101e6a5..749b410e16 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -67,7 +67,8 @@ CLUSTER [VERBOSE]
- CLUSTER without any parameter reclusters all the
+ CLUSTER without a
+ table_name reclusters all the
previously-clustered tables in the current database that the calling user
owns or has the MAINTAIN privilege for, or all such tables
if called by a superuser or a role with privileges of the
@@ -134,6 +135,15 @@ CLUSTER [VERBOSE]
Notes
+
+To cluster a table, one must have the MAINTAIN privilege
+on the table or be the table's owner, a superuser, or a role with
+privileges of the
+pg_maintain
+role. Database-wide clusters and clusters on partitioned tables will skip
+over any tables that the calling user does not have permission to cluster.
+
+
In cases where you are accessing single rows randomly
within a table, the actual order of the data in the
@@ -202,7 +212,8 @@ CLUSTER [VERBOSE]
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. CLUSTER on a
+partitioned table cannot be executed inside a transaction block.
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index f11691aff7..55b699ef05 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -80,6 +80,7 @@ static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
static List *get_tables_to_cluster(MemoryContext cluster_context);
static List *get_tables_to_cluster_partitioned(MemoryContext cluster_context,
Oid indexOid);
+static bool cluster_is_permitted_for_relation(Oid relid, Oid userid);
/*---
@@ -366,8 +367,7 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
if (recheck)
{
/* Check that the user still has privileges for the relation */
- if (!object_ownercheck(RelationRelationId, tableOid, save_userid) &&
- pg_class_aclcheck(tableOid, save_userid, ACL_MAINTAIN) != ACLCHECK_OK)
+ if (!cluster_is_permitted_for_relation(tableOid, save_userid))
{
relation_close(OldHeap, AccessExclusiveLock);
goto out;
@@ -1645,8 +1645,7 @@ get_tables_to_cluster(MemoryContext cluster_context)
index = (Form_pg_index) GETSTRUCT(indexTuple);
- if (!object_ownercheck(RelationRelationId, index->indrelid, GetUserId()) &&
- pg_class_aclcheck(index->indrelid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK)
+ if (!cluster_is_permitted_for_relation(index->indrelid, GetUserId()))
continue;
/* Use a permanent memory context for the result list */
@@ -1695,10 +1694,7 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
continue;
/* Silently skip partitions which the user has no access to. */
- if (!object_ownercheck(RelationRelationId, relid, GetUserId()) &&
- pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK &&
- (!object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) ||
- IsSharedRelation(relid)))
+ if (!cluster_is_permitted_for_relation(relid, GetUserId()))
continue;
/* Use a permanent memory context for the result list */
@@ -1714,3 +1710,16 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
return rtcs;
}
+
+static bool
+cluster_is_permitted_for_relation(Oid relid, Oid userid)
+{
+ if (object_ownercheck(RelationRelationId, relid, userid) ||
+ pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK)
+ return true;
+
+ ereport(WARNING,
+ (errmsg("permission denied to cluster \"%s\", skipping it",
+ get_rel_name(relid;
+ return false;
+}
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index 542c2e098c..27a5dff5d4 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -352,7 +352,9 @@ INSERT INTO clstr_3 VALUES (1);
-- this user can only cluster clstr_1 and clstr_3, but the latter
-- has not been clustered
SET SESSION AUTHORIZATION regress_clstr_user;
+SET client_min_messages = ERROR; -- order of "skipping" warnings may vary
CLUSTER;
+RESET client_min_messages;
SELECT * FROM
Re: fix and document CLUSTER privileges
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
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
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
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
diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index 145101e6a5..9ca66cd2ee 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -67,7 +67,8 @@ CLUSTER [VERBOSE]
- CLUSTER without any parameter reclusters all the
+ CLUSTER without a
+ table_name reclusters all the
previously-clustered tables in the current database that the calling user
owns or has the MAINTAIN privilege for, or all such tables
if called by a superuser or a role with privileges of the
@@ -134,6 +135,16 @@ CLUSTER [VERBOSE]
Notes
+
+To cluster a table, one must have the MAINTAIN privilege
+on the table or be the table's owner, a superuser, or a role with
+privileges of the
+pg_maintain
+role. Database-wide clusters and clusters on partitioned tables will
+silently skip over any tables that the calling user does not have
+permission to cluster.
+
+
In cases where you are accessing single rows randomly
within a table, the actual order of the data in the
@@ -202,7 +213,8 @@ CLUSTER [VERBOSE]
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. CLUSTER on a
+partitioned table cannot be executed inside a transaction block.
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 8966b75bd1..14328abfa6 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -80,6 +80,7 @@ static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
static List *get_tables_to_cluster(MemoryContext cluster_context);
static List *get_tables_to_cluster_partitioned(MemoryContext cluster_context,
Oid indexOid);
+static bool cluster_is_permitted_for_relation(Oid relid, Oid userid);
/*---
@@ -366,8 +367,7 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
if (recheck)
{
/* Check that the user still has privileges for the relation */
- if (!object_ownercheck(RelationRelationId, tableOid, save_userid) &&
- pg_class_aclcheck(tableOid, save_userid, ACL_MAINTAIN) != ACLCHECK_OK)
+ if (!cluster_is_permitted_for_relation(tableOid, save_userid))
{
relation_close(OldHeap, AccessExclusiveLock);
goto out;
@@ -1646,8 +1646,7 @@ get_tables_to_cluster(MemoryContext cluster_context)
index = (Form_pg_index) GETSTRUCT(indexTuple);
- if (!object_ownercheck(RelationRelationId, index->indrelid, GetUserId()) &&
- pg_class_aclcheck(index->indrelid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK)
+ if (!cluster_is_permitted_for_relation(index->indrelid, GetUserId()))
continue;
/* Use a permanent memory context for the result list */
@@ -1696,10 +1695,7 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
continue;
/* Silently skip partitions which the user has no access to. */
- if (!object_ownercheck(RelationRelationId, relid, GetUserId()) &&
- pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK &&
- (!object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) ||
- IsSharedRelation(relid)))
+ if (!cluster_is_permitted_for_relation(relid, GetUserId()))
continue;
/* Use a permanent memory context for the result list */
@@ -1715,3 +1711,10 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
return rtcs;
}
+
+static bool
+cluster_is_permitted_for_relation(Oid relid, Oid userid)
+{
+ return object_ownercheck(RelationRelationId, relid, userid) ||
+ pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK;
+}
Re: fix and document CLUSTER privileges
On Thu, Dec 08, 2022 at 04:08:40PM -0500, Robert Haas wrote: > On Thu, Dec 8, 2022 at 1:13 PM Nathan Bossart > 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 diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml index 145101e6a5..d6b2651657 100644 --- a/doc/src/sgml/ref/cluster.sgml +++ b/doc/src/sgml/ref/cluster.sgml @@ -67,7 +67,8 @@ CLUSTER [VERBOSE] - CLUSTER without any parameter reclusters all the + CLUSTER without a + table_name reclusters all the previously-clustered tables in the current database that the calling user owns or has the MAINTAIN privilege for, or all such tables if called by a superuser or a role with privileges of the @@ -134,6 +135,16 @@ CLUSTER [VERBOSE] Notes + +To cluster a table, one must have the MAINTAIN privilege +on the table or be the table's owner, a superuser, or a role with +privileges of the +pg_maintain +role. Database-wide clusters and clusters on partitioned tables will +silently skip over any tables that the calling user does not have +permission to cluster. + + In cases where you are accessing single rows randomly within a table, the actual order of the data in the diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 8966b75bd1..8140a90699 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -1697,9 +1697,7 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid) /* Silently skip partitions which the user has no access to. */ if (!object_ownercheck(RelationRelationId, relid, GetUserId()) && - pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && - (!object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) || - IsSharedRelation(relid))) + pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK) continue; /* Use a permanent memory context for the result list */
Re: fix and document CLUSTER privileges
On Thu, Dec 8, 2022 at 1:13 PM Nathan Bossart 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
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
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
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
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
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
fix and document CLUSTER privileges
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 diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml index c37f4236f1..feb61e6ec2 100644 --- a/doc/src/sgml/ref/cluster.sgml +++ b/doc/src/sgml/ref/cluster.sgml @@ -67,7 +67,8 @@ CLUSTER [VERBOSE] - CLUSTER without any parameter reclusters all the + CLUSTER without a + table_name reclusters all the previously-clustered tables in the current database that the calling user owns, or all such tables if called by a superuser. This form of CLUSTER cannot be executed inside a transaction @@ -132,6 +133,13 @@ CLUSTER [VERBOSE] Notes + +To cluster a table, one must be the table's owner or a superuser. +Database-wide clusters and clusters on partitioned tables will silently +skip over any tables that the calling user does not have permission to +cluster. + + In cases where you are accessing single rows randomly within a table, the actual order of the data in the diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 07e091bb87..a8a51cc674 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -1693,9 +1693,7 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid) continue; /* Silently skip partitions which the user has no access to. */ - if (!object_ownercheck(RelationRelationId, relid, GetUserId()) && - (!object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) || - IsSharedRelation(relid))) + if (!object_ownercheck(RelationRelationId, relid, GetUserId())) continue; /* Use a permanent memory context for the result list */
