Thread: 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
Attachment
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
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
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
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
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
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
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
Attachment
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
Attachment
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
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
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
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
Attachment
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
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
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
Attachment
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
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
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
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
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
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