agora inbox for [email protected]help / color / mirror / Atom feed
[PATCH v6 1/7] pg_dump: make CLUSTER ON a separate dump object.. 8+ messages / 2 participants [nested] [flat]
* [PATCH v6 1/7] pg_dump: make CLUSTER ON a separate dump object.. @ 2020-11-26 20:37 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Justin Pryzby @ 2020-11-26 20:37 UTC (permalink / raw) ..since it needs to be restored after any child indexes are restored *and attached*. The order needs to be: 1) restore child and parent index (order doesn't matter); 2) attach child index; 3) set cluster on child and parent index (order doesn't matter); --- src/bin/pg_dump/pg_dump.c | 86 ++++++++++++++++++++++++++-------- src/bin/pg_dump/pg_dump.h | 8 ++++ src/bin/pg_dump/pg_dump_sort.c | 8 ++++ 3 files changed, 82 insertions(+), 20 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 798d14580e..e6526392e5 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -208,6 +208,7 @@ static void dumpSequence(Archive *fout, TableInfo *tbinfo); static void dumpSequenceData(Archive *fout, TableDataInfo *tdinfo); static void dumpIndex(Archive *fout, IndxInfo *indxinfo); static void dumpIndexAttach(Archive *fout, IndexAttachInfo *attachinfo); +static void dumpIndexClusterOn(Archive *fout, IndexClusterInfo *clusterinfo); static void dumpStatisticsExt(Archive *fout, StatsExtInfo *statsextinfo); static void dumpConstraint(Archive *fout, ConstraintInfo *coninfo); static void dumpTableConstraintComment(Archive *fout, ConstraintInfo *coninfo); @@ -7092,6 +7093,11 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables) i_inddependcollversions; int ntups; + int ncluster = 0; + IndexClusterInfo *clusterinfo; + clusterinfo = (IndexClusterInfo *) + pg_malloc0(numTables * sizeof(IndexClusterInfo)); + for (i = 0; i < numTables; i++) { TableInfo *tbinfo = &tblinfo[i]; @@ -7471,6 +7477,27 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables) /* Plain secondary index */ indxinfo[j].indexconstraint = 0; } + + /* Record each table's CLUSTERed index, if any */ + if (indxinfo[j].indisclustered) + { + IndxInfo *index = &indxinfo[j]; + IndexClusterInfo *cluster = &clusterinfo[ncluster]; + + cluster->dobj.objType = DO_INDEX_CLUSTER_ON; + cluster->dobj.catId.tableoid = 0; + cluster->dobj.catId.oid = 0; + AssignDumpId(&cluster->dobj); + cluster->dobj.name = pg_strdup(index->dobj.name); + cluster->dobj.namespace = index->indextable->dobj.namespace; + cluster->index = index; + cluster->indextable = &tblinfo[i]; + + /* The CLUSTER ON depends on its index.. */ + addObjectDependency(&cluster->dobj, index->dobj.dumpId); + + ncluster++; + } } PQclear(res); @@ -10296,6 +10323,9 @@ dumpDumpableObject(Archive *fout, DumpableObject *dobj) case DO_SUBSCRIPTION: dumpSubscription(fout, (SubscriptionInfo *) dobj); break; + case DO_INDEX_CLUSTER_ON: + dumpIndexClusterOn(fout, (IndexClusterInfo *) dobj); + break; case DO_PRE_DATA_BOUNDARY: case DO_POST_DATA_BOUNDARY: /* never dumped, nothing to do */ @@ -16516,6 +16546,41 @@ getAttrName(int attrnum, TableInfo *tblInfo) return NULL; /* keep compiler quiet */ } +/* + * dumpIndexClusterOn + * record that the index is clustered. + */ +static void +dumpIndexClusterOn(Archive *fout, IndexClusterInfo *clusterinfo) +{ + DumpOptions *dopt = fout->dopt; + TableInfo *tbinfo = clusterinfo->indextable; + char *qindxname; + PQExpBuffer q; + + if (dopt->dataOnly) + return; + + q = createPQExpBuffer(); + qindxname = pg_strdup(fmtId(clusterinfo->dobj.name)); + + /* index name is not qualified in this syntax */ + appendPQExpBuffer(q, "\nALTER TABLE %s CLUSTER ON %s;\n", + fmtQualifiedDumpable(tbinfo), qindxname); + + if (clusterinfo->dobj.dump & DUMP_COMPONENT_DEFINITION) + ArchiveEntry(fout, clusterinfo->dobj.catId, clusterinfo->dobj.dumpId, + ARCHIVE_OPTS(.tag = clusterinfo->dobj.name, + .namespace = tbinfo->dobj.namespace->dobj.name, + .owner = tbinfo->rolname, + .description = "INDEX CLUSTER ON", + .section = SECTION_POST_DATA, + .createStmt = q->data)); + + destroyPQExpBuffer(q); + free(qindxname); +} + /* * dumpIndex * write out to fout a user-defined index @@ -16570,16 +16635,6 @@ dumpIndex(Archive *fout, IndxInfo *indxinfo) * similar code in dumpConstraint! */ - /* If the index is clustered, we need to record that. */ - if (indxinfo->indisclustered) - { - appendPQExpBuffer(q, "\nALTER TABLE %s CLUSTER", - fmtQualifiedDumpable(tbinfo)); - /* index name is not qualified in this syntax */ - appendPQExpBuffer(q, " ON %s;\n", - qindxname); - } - /* * If the index has any statistics on some of its columns, generate * the associated ALTER INDEX queries. @@ -16906,16 +16961,6 @@ dumpConstraint(Archive *fout, ConstraintInfo *coninfo) * similar code in dumpIndex! */ - /* If the index is clustered, we need to record that. */ - if (indxinfo->indisclustered) - { - appendPQExpBuffer(q, "\nALTER TABLE %s CLUSTER", - fmtQualifiedDumpable(tbinfo)); - /* index name is not qualified in this syntax */ - appendPQExpBuffer(q, " ON %s;\n", - fmtId(indxinfo->dobj.name)); - } - /* If the index defines identity, we need to record that. */ if (indxinfo->indisreplident) { @@ -18421,6 +18466,7 @@ addBoundaryDependencies(DumpableObject **dobjs, int numObjs, break; case DO_INDEX: case DO_INDEX_ATTACH: + case DO_INDEX_CLUSTER_ON: case DO_STATSEXT: case DO_REFRESH_MATVIEW: case DO_TRIGGER: diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h index 1290f9659b..57def4c009 100644 --- a/src/bin/pg_dump/pg_dump.h +++ b/src/bin/pg_dump/pg_dump.h @@ -54,6 +54,7 @@ typedef enum DO_ATTRDEF, DO_INDEX, DO_INDEX_ATTACH, + DO_INDEX_CLUSTER_ON, DO_STATSEXT, DO_RULE, DO_TRIGGER, @@ -386,6 +387,13 @@ typedef struct _indxInfo DumpId indexconstraint; } IndxInfo; +typedef struct _indexClusterInfo +{ + DumpableObject dobj; + TableInfo *indextable; /* link to table the index is for */ + IndxInfo *index; /* link to index itself */ +} IndexClusterInfo; + typedef struct _indexAttachInfo { DumpableObject dobj; diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c index 46461fb6a1..dd5b233196 100644 --- a/src/bin/pg_dump/pg_dump_sort.c +++ b/src/bin/pg_dump/pg_dump_sort.c @@ -75,6 +75,7 @@ enum dbObjectTypePriorities PRIO_CONSTRAINT, PRIO_INDEX, PRIO_INDEX_ATTACH, + PRIO_INDEX_CLUSTER_ON, PRIO_STATSEXT, PRIO_RULE, PRIO_TRIGGER, @@ -108,6 +109,7 @@ static const int dbObjectTypePriority[] = PRIO_ATTRDEF, /* DO_ATTRDEF */ PRIO_INDEX, /* DO_INDEX */ PRIO_INDEX_ATTACH, /* DO_INDEX_ATTACH */ + PRIO_INDEX_CLUSTER_ON, /* DO_INDEX_CLUSTER_ON */ PRIO_STATSEXT, /* DO_STATSEXT */ PRIO_RULE, /* DO_RULE */ PRIO_TRIGGER, /* DO_TRIGGER */ @@ -136,6 +138,7 @@ static const int dbObjectTypePriority[] = PRIO_PUBLICATION, /* DO_PUBLICATION */ PRIO_PUBLICATION_REL, /* DO_PUBLICATION_REL */ PRIO_SUBSCRIPTION /* DO_SUBSCRIPTION */ + }; StaticAssertDecl(lengthof(dbObjectTypePriority) == (DO_SUBSCRIPTION + 1), @@ -1348,6 +1351,11 @@ describeDumpableObject(DumpableObject *obj, char *buf, int bufsize) "INDEX ATTACH %s (ID %d)", obj->name, obj->dumpId); return; + case DO_INDEX_CLUSTER_ON: + snprintf(buf, bufsize, + "INDEX CLUSTER ON %s (ID %d)", + obj->name, obj->dumpId); + return; case DO_STATSEXT: snprintf(buf, bufsize, "STATISTICS %s (ID %d OID %u)", -- 2.17.0 --FsscpQKzF/jJk6ya Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v6-0002-Implement-CLUSTER-of-partitioned-table.patch" ^ permalink raw reply [nested|flat] 8+ messages in thread
* [PATCH v1 1/2] partial revert of ff9618e82a @ 2023-06-14 17:54 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Nathan Bossart @ 2023-06-14 17:54 UTC (permalink / raw) --- doc/src/sgml/ref/analyze.sgml | 5 +--- doc/src/sgml/ref/cluster.sgml | 6 +--- doc/src/sgml/ref/lock.sgml | 5 +--- doc/src/sgml/ref/reindex.sgml | 6 +--- doc/src/sgml/ref/vacuum.sgml | 5 +--- src/backend/commands/cluster.c | 10 ++----- src/backend/commands/indexcmds.c | 19 +++++------- src/backend/commands/lockcmds.c | 8 ----- src/backend/commands/tablecmds.c | 29 +------------------ src/backend/commands/vacuum.c | 6 +--- src/include/commands/tablecmds.h | 1 - .../expected/cluster-conflict-partition.out | 14 ++++----- .../specs/cluster-conflict-partition.spec | 7 +++-- src/test/regress/expected/cluster.out | 3 +- src/test/regress/expected/vacuum.out | 18 ++++++++++++ 15 files changed, 49 insertions(+), 93 deletions(-) diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml index 20c6f9939f..30a893230e 100644 --- a/doc/src/sgml/ref/analyze.sgml +++ b/doc/src/sgml/ref/analyze.sgml @@ -190,10 +190,7 @@ ANALYZE [ VERBOSE ] [ <replaceable class="parameter">table_and_columns</replacea analyze all tables in their databases, except shared catalogs. (The restriction for shared catalogs means that a true database-wide <command>ANALYZE</command> can only be performed by superusers and roles - with privileges of <literal>pg_maintain</literal>.) If a role has - permission to <command>ANALYZE</command> a partitioned table, it is also - permitted to <command>ANALYZE</command> each of its partitions, regardless - of whether the role has the aforementioned privileges on the partition. + with privileges of <literal>pg_maintain</literal>.) <command>ANALYZE</command> will skip over any tables that the calling user does not have permission to analyze. </para> diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml index 29f0f1fd90..c768175499 100644 --- a/doc/src/sgml/ref/cluster.sgml +++ b/doc/src/sgml/ref/cluster.sgml @@ -137,11 +137,7 @@ CLUSTER [VERBOSE] on the table or be the table's owner, a superuser, or a role with privileges of the <link linkend="predefined-roles-table"><literal>pg_maintain</literal></link> - role. If a role has permission to <command>CLUSTER</command> a partitioned - table, it is also permitted to <command>CLUSTER</command> each of its - partitions, regardless of whether the role has the aforementioned - privileges on the partition. <command>CLUSTER</command> will skip over any - tables that the calling user does not have permission to cluster. + role. </para> <para> diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml index 5b3b2b793a..8524182211 100644 --- a/doc/src/sgml/ref/lock.sgml +++ b/doc/src/sgml/ref/lock.sgml @@ -177,10 +177,7 @@ LOCK [ TABLE ] [ ONLY ] <replaceable class="parameter">name</replaceable> [ * ] MODE</literal> (or a less-conflicting mode as described in <xref linkend="explicit-locking"/>) is permitted. If a user has <literal>SELECT</literal> privileges on the table, <literal>ACCESS SHARE - MODE</literal> is permitted. If a role has permission to lock a - partitioned table, it is also permitted to lock each of its partitions, - regardless of whether the role has the aforementioned privileges on the - partition. + MODE</literal> is permitted. </para> <para> diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml index 71455dfdc7..23f8c7630b 100644 --- a/doc/src/sgml/ref/reindex.sgml +++ b/doc/src/sgml/ref/reindex.sgml @@ -306,11 +306,7 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { DA indexes on shared catalogs will be skipped unless the user owns the catalog (which typically won't be the case), has privileges of the <literal>pg_maintain</literal> role, or has the <literal>MAINTAIN</literal> - 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. </para> <para> diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml index 57bc4c23ec..445325e14c 100644 --- a/doc/src/sgml/ref/vacuum.sgml +++ b/doc/src/sgml/ref/vacuum.sgml @@ -452,10 +452,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet vacuum all tables in their databases, except shared catalogs. (The restriction for shared catalogs means that a true database-wide <command>VACUUM</command> can only be performed by superusers and roles - with privileges of <literal>pg_maintain</literal>.) If a role has - permission to <command>VACUUM</command> a partitioned table, it is also - permitted to <command>VACUUM</command> each of its partitions, regardless - of whether the role has the aforementioned privileges on the partition. + with privileges of <literal>pg_maintain</literal>.) <command>VACUUM</command> will skip over any tables that the calling user does not have permission to vacuum. </para> diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 369fea7c04..38834356b9 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -1693,11 +1693,8 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid) if (get_rel_relkind(indexrelid) != RELKIND_INDEX) continue; - /* - * 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; /* Use a permanent memory context for the result list */ old_context = MemoryContextSwitchTo(cluster_context); @@ -1720,8 +1717,7 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid) static bool cluster_is_permitted_for_relation(Oid relid, Oid userid) { - if (pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK || - has_partition_ancestor_privs(relid, userid, ACL_MAINTAIN)) + if (pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK) return true; ereport(WARNING, diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index a5168c9f09..56bba91c08 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -2854,8 +2854,7 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation, /* Check permissions */ table_oid = IndexGetRelation(relId, true); if (OidIsValid(table_oid) && - pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && - !has_partition_ancestor_privs(table_oid, GetUserId(), ACL_MAINTAIN)) + pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK) aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_INDEX, relation->relname); @@ -3065,17 +3064,15 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, /* * The table can be reindexed if the user has been granted MAINTAIN on - * the table or one of its partition ancestors or the user is a - * superuser, the table owner, or the database/schema owner (but in - * the latter case, only if it's not a shared relation). - * pg_class_aclcheck includes the superuser case, and depending on - * objectKind we already know that the user has permission to run - * REINDEX on this database or schema per the permission checks at the - * beginning of this routine. + * the table or the user is a superuser, the table owner, or the + * database/schema owner (but in the latter case, only if it's not a + * shared relation). pg_class_aclcheck includes the superuser case, + * and depending on objectKind we already know that the user has + * permission to run REINDEX on this database or schema per the + * permission checks at the beginning of this routine. */ if (classtuple->relisshared && - pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && - !has_partition_ancestor_privs(relid, GetUserId(), ACL_MAINTAIN)) + pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK) continue; /* diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c index 43c7d7f4bb..92662cbbc8 100644 --- a/src/backend/commands/lockcmds.c +++ b/src/backend/commands/lockcmds.c @@ -19,7 +19,6 @@ #include "catalog/namespace.h" #include "catalog/pg_inherits.h" #include "commands/lockcmds.h" -#include "commands/tablecmds.h" #include "miscadmin.h" #include "nodes/nodeFuncs.h" #include "parser/parse_clause.h" @@ -297,12 +296,5 @@ LockTableAclCheck(Oid reloid, LOCKMODE lockmode, Oid userid) aclresult = pg_class_aclcheck(reloid, userid, aclmask); - /* - * If this is a partition, check permissions of its ancestors if needed. - */ - if (aclresult != ACLCHECK_OK && - has_partition_ancestor_privs(reloid, userid, ACL_MAINTAIN)) - aclresult = ACLCHECK_OK; - return aclresult; } diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 4d49d70c33..29eece1c2c 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -17006,38 +17006,11 @@ RangeVarCallbackMaintainsTable(const RangeVar *relation, errmsg("\"%s\" is not a table or materialized view", relation->relname))); /* Check permissions */ - if (pg_class_aclcheck(relId, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && - !has_partition_ancestor_privs(relId, GetUserId(), ACL_MAINTAIN)) + if (pg_class_aclcheck(relId, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK) aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_TABLE, relation->relname); } -/* - * If relid is a partition, returns whether userid has any of the privileges - * specified in acl on any of its ancestors. Otherwise, returns false. - */ -bool -has_partition_ancestor_privs(Oid relid, Oid userid, AclMode acl) -{ - List *ancestors; - ListCell *lc; - - if (!get_rel_relispartition(relid)) - return false; - - ancestors = get_partition_ancestors(relid); - foreach(lc, ancestors) - { - Oid ancestor = lfirst_oid(lc); - - if (OidIsValid(ancestor) && - pg_class_aclcheck(ancestor, userid, acl) == ACLCHECK_OK) - return true; - } - - return false; -} - /* * Callback to RangeVarGetRelidExtended() for TRUNCATE processing. */ diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index a843f9ad92..2e41a31173 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -41,7 +41,6 @@ #include "catalog/pg_namespace.h" #include "commands/cluster.h" #include "commands/defrem.h" -#include "commands/tablecmds.h" #include "commands/vacuum.h" #include "miscadmin.h" #include "nodes/makefuncs.h" @@ -719,13 +718,10 @@ vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple, * - the role owns the relation * - the role owns the current database and the relation is not shared * - the role has been granted the MAINTAIN privilege on the relation - * - the role has privileges to vacuum/analyze any of the relation's - * partition ancestors *---------- */ if ((object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) && !reltuple->relisshared) || - pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) == ACLCHECK_OK || - has_partition_ancestor_privs(relid, GetUserId(), ACL_MAINTAIN)) + pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) == ACLCHECK_OK) return true; relname = NameStr(reltuple->relname); diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h index 17b9404937..250d89ff88 100644 --- a/src/include/commands/tablecmds.h +++ b/src/include/commands/tablecmds.h @@ -99,7 +99,6 @@ extern void AtEOSubXact_on_commit_actions(bool isCommit, extern void RangeVarCallbackMaintainsTable(const RangeVar *relation, Oid relId, Oid oldRelId, void *arg); -extern bool has_partition_ancestor_privs(Oid relid, Oid userid, AclMode acl); extern void RangeVarCallbackOwnsRelation(const RangeVar *relation, Oid relId, Oid oldRelId, void *arg); diff --git a/src/test/isolation/expected/cluster-conflict-partition.out b/src/test/isolation/expected/cluster-conflict-partition.out index 8d21276996..7be9e56ef1 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; <waiting ...> step s1_commit: COMMIT; step s2_cluster: <... completed> @@ -11,7 +11,7 @@ step s2_reset: RESET ROLE; starting permutation: s1_begin s2_auth s1_lock_parent s2_cluster s1_commit s2_reset step s1_begin: BEGIN; -step s2_auth: SET ROLE regress_cluster_part; +step s2_auth: SET ROLE regress_cluster_part; SET client_min_messages = ERROR; step s1_lock_parent: LOCK cluster_part_tab IN SHARE UPDATE EXCLUSIVE MODE; step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...> step s1_commit: COMMIT; @@ -21,17 +21,15 @@ step s2_reset: RESET ROLE; starting permutation: s1_begin s1_lock_child s2_auth s2_cluster s1_commit s2_reset step s1_begin: BEGIN; step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; -step s2_auth: SET ROLE regress_cluster_part; -step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...> +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> step s2_reset: RESET ROLE; starting permutation: s1_begin s2_auth s1_lock_child s2_cluster s1_commit s2_reset step s1_begin: BEGIN; -step s2_auth: SET ROLE regress_cluster_part; +step s2_auth: SET ROLE regress_cluster_part; SET client_min_messages = ERROR; step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; -step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...> +step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; step s1_commit: COMMIT; -step s2_cluster: <... completed> step s2_reset: RESET ROLE; diff --git a/src/test/isolation/specs/cluster-conflict-partition.spec b/src/test/isolation/specs/cluster-conflict-partition.spec index ae38cb4ee3..4d38a7f49a 100644 --- a/src/test/isolation/specs/cluster-conflict-partition.spec +++ b/src/test/isolation/specs/cluster-conflict-partition.spec @@ -23,12 +23,15 @@ step s1_lock_child { LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; step s1_commit { COMMIT; } session s2 -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 s2_reset { RESET ROLE; } -# CLUSTER waits if locked, passes for all cases. +# CLUSTER on the parent waits if locked, passes for all cases. permutation s1_begin s1_lock_parent s2_auth s2_cluster s1_commit s2_reset permutation s1_begin s2_auth s1_lock_parent s2_cluster s1_commit s2_reset + +# When taking a lock on a partition leaf, CLUSTER on the parent skips +# the leaf, passes for all cases. permutation s1_begin s1_lock_child s2_auth s2_cluster s1_commit s2_reset permutation s1_begin s2_auth s1_lock_child s2_cluster s1_commit s2_reset diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out index 2eec483eaa..27a5dff5d4 100644 --- a/src/test/regress/expected/cluster.out +++ b/src/test/regress/expected/cluster.out @@ -508,6 +508,7 @@ CREATE TEMP TABLE ptnowner_oldnodes AS JOIN pg_class AS c ON c.oid=tree.relid; SET SESSION AUTHORIZATION regress_ptnowner; CLUSTER ptnowner USING ptnowner_i_idx; +WARNING: permission denied to cluster "ptnowner2", skipping it RESET SESSION AUTHORIZATION; SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a JOIN ptnowner_oldnodes b USING (oid) ORDER BY a.relname COLLATE "C"; @@ -515,7 +516,7 @@ SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a -----------+---------- ptnowner | t ptnowner1 | f - ptnowner2 | f + ptnowner2 | t (3 rows) DROP TABLE ptnowner; diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out index 41e020cf20..4def90b805 100644 --- a/src/test/regress/expected/vacuum.out +++ b/src/test/regress/expected/vacuum.out @@ -442,14 +442,20 @@ ALTER TABLE vacowned_parted OWNER TO regress_vacuum; ALTER TABLE vacowned_part1 OWNER TO regress_vacuum; SET ROLE regress_vacuum; VACUUM vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM vacowned_part1; VACUUM vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it ANALYZE vacowned_parted; +WARNING: permission denied to analyze "vacowned_part2", skipping it ANALYZE vacowned_part1; ANALYZE vacowned_part2; +WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; VACUUM (ANALYZE) vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it RESET ROLE; -- Only one partition owned by other user. ALTER TABLE vacowned_parted OWNER TO CURRENT_USER; @@ -478,14 +484,26 @@ ALTER TABLE vacowned_parted OWNER TO regress_vacuum; ALTER TABLE vacowned_part1 OWNER TO CURRENT_USER; SET ROLE regress_vacuum; VACUUM vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part1", skipping it +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM vacowned_part1; +WARNING: permission denied to vacuum "vacowned_part1", skipping it VACUUM vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it ANALYZE vacowned_parted; +WARNING: permission denied to analyze "vacowned_part1", skipping it +WARNING: permission denied to analyze "vacowned_part2", skipping it ANALYZE vacowned_part1; +WARNING: permission denied to analyze "vacowned_part1", skipping it ANALYZE vacowned_part2; +WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part1", skipping it +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; +WARNING: permission denied to vacuum "vacowned_part1", skipping it VACUUM (ANALYZE) vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it RESET ROLE; DROP TABLE vacowned; DROP TABLE vacowned_parted; -- 2.25.1 --a8Wt8u1KmwUX3Y2C Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v1-0002-convert-boolean-parameter-into-a-VACOPT_-option.patch" ^ permalink raw reply [nested|flat] 8+ messages in thread
* [PATCH v3 1/2] partial revert of ff9618e82a @ 2023-06-14 17:54 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Nathan Bossart @ 2023-06-14 17:54 UTC (permalink / raw) --- doc/src/sgml/ref/analyze.sgml | 11 +------ doc/src/sgml/ref/cluster.sgml | 9 +----- doc/src/sgml/ref/lock.sgml | 11 ++----- .../sgml/ref/refresh_materialized_view.sgml | 6 ++-- doc/src/sgml/ref/reindex.sgml | 27 +++++++---------- doc/src/sgml/ref/vacuum.sgml | 11 +------ doc/src/sgml/user-manag.sgml | 3 +- src/backend/commands/cluster.c | 10 ++----- src/backend/commands/indexcmds.c | 17 ++++------- src/backend/commands/lockcmds.c | 8 ----- src/backend/commands/tablecmds.c | 29 +------------------ src/backend/commands/vacuum.c | 6 +--- src/include/commands/tablecmds.h | 1 - .../expected/cluster-conflict-partition.out | 14 ++++----- .../specs/cluster-conflict-partition.spec | 7 +++-- src/test/regress/expected/cluster.out | 3 +- src/test/regress/expected/vacuum.out | 18 ++++++++++++ 17 files changed, 61 insertions(+), 130 deletions(-) diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml index 20c6f9939f..954491b5df 100644 --- a/doc/src/sgml/ref/analyze.sgml +++ b/doc/src/sgml/ref/analyze.sgml @@ -183,17 +183,8 @@ ANALYZE [ VERBOSE ] [ <replaceable class="parameter">table_and_columns</replacea <para> To analyze a table, one must ordinarily have the <literal>MAINTAIN</literal> - privilege on the table or be the table's owner, a superuser, or a role with - privileges of the - <link linkend="predefined-roles-table"><literal>pg_maintain</literal></link> - role. However, database owners are allowed to + privilege on the table. However, database owners are allowed to analyze all tables in their databases, except shared catalogs. - (The restriction for shared catalogs means that a true database-wide - <command>ANALYZE</command> can only be performed by superusers and roles - with privileges of <literal>pg_maintain</literal>.) If a role has - permission to <command>ANALYZE</command> a partitioned table, it is also - permitted to <command>ANALYZE</command> each of its partitions, regardless - of whether the role has the aforementioned privileges on the partition. <command>ANALYZE</command> will skip over any tables that the calling user does not have permission to analyze. </para> diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml index 29f0f1fd90..06f3d269e6 100644 --- a/doc/src/sgml/ref/cluster.sgml +++ b/doc/src/sgml/ref/cluster.sgml @@ -134,14 +134,7 @@ CLUSTER [VERBOSE] <para> To cluster a table, one must have the <literal>MAINTAIN</literal> privilege - on the table or be the table's owner, a superuser, or a role with - privileges of the - <link linkend="predefined-roles-table"><literal>pg_maintain</literal></link> - role. If a role has permission to <command>CLUSTER</command> a partitioned - table, it is also permitted to <command>CLUSTER</command> each of its - partitions, regardless of whether the role has the aforementioned - privileges on the partition. <command>CLUSTER</command> will skip over any - tables that the calling user does not have permission to cluster. + on the table. </para> <para> diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml index 5b3b2b793a..070855da18 100644 --- a/doc/src/sgml/ref/lock.sgml +++ b/doc/src/sgml/ref/lock.sgml @@ -166,10 +166,8 @@ LOCK [ TABLE ] [ ONLY ] <replaceable class="parameter">name</replaceable> [ * ] <para> To lock a table, the user must have the right privilege for the specified - <replaceable class="parameter">lockmode</replaceable>, or be the table's - owner, a superuser, or a role with privileges of the <link - linkend="predefined-roles-table"><literal>pg_maintain</literal></link> - role. If the user has <literal>MAINTAIN</literal>, + <replaceable class="parameter">lockmode</replaceable>. + If the user has <literal>MAINTAIN</literal>, <literal>UPDATE</literal>, <literal>DELETE</literal>, or <literal>TRUNCATE</literal> privileges on the table, any <replaceable class="parameter">lockmode</replaceable> is permitted. If the user has @@ -177,10 +175,7 @@ LOCK [ TABLE ] [ ONLY ] <replaceable class="parameter">name</replaceable> [ * ] MODE</literal> (or a less-conflicting mode as described in <xref linkend="explicit-locking"/>) is permitted. If a user has <literal>SELECT</literal> privileges on the table, <literal>ACCESS SHARE - MODE</literal> is permitted. If a role has permission to lock a - partitioned table, it is also permitted to lock each of its partitions, - regardless of whether the role has the aforementioned privileges on the - partition. + MODE</literal> is permitted. </para> <para> diff --git a/doc/src/sgml/ref/refresh_materialized_view.sgml b/doc/src/sgml/ref/refresh_materialized_view.sgml index 4d79b6ae7f..19737668cd 100644 --- a/doc/src/sgml/ref/refresh_materialized_view.sgml +++ b/doc/src/sgml/ref/refresh_materialized_view.sgml @@ -31,10 +31,8 @@ REFRESH MATERIALIZED VIEW [ CONCURRENTLY ] <replaceable class="parameter">name</ <para> <command>REFRESH MATERIALIZED VIEW</command> completely replaces the - contents of a materialized view. To execute this command you must be the - owner of the materialized view, have privileges of the - <link linkend="predefined-roles-table"><literal>pg_maintain</literal></link> - role, or have the <literal>MAINTAIN</literal> + contents of a materialized view. To execute this command you must have the + <literal>MAINTAIN</literal> privilege on the materialized view. The old contents are discarded. If <literal>WITH DATA</literal> is specified (or defaults) the backing query is executed to provide the new data, and the materialized view is left in a diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml index 71455dfdc7..583553b8a3 100644 --- a/doc/src/sgml/ref/reindex.sgml +++ b/doc/src/sgml/ref/reindex.sgml @@ -292,25 +292,18 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { DA </para> <para> - Reindexing a single index or table requires being the owner of that - index or table, having privileges of the + Reindexing a single index or table requires having the + <literal>MAINTAIN</literal> privilege on the table. Reindexing a schema or + database requires being the owner of that schema or database or having + privileges of the <link linkend="predefined-roles-table"><literal>pg_maintain</literal></link> - role, or having the <literal>MAINTAIN</literal> privilege on the - table. Reindexing a schema or database requires being the - owner of that schema or database or having privileges of the - <literal>pg_maintain</literal> role. Note specifically that it's thus + role. Note specifically that it's thus possible for non-superusers to rebuild indexes of tables owned by - other users. However, as a special exception, when - <command>REINDEX DATABASE</command>, <command>REINDEX SCHEMA</command> - or <command>REINDEX SYSTEM</command> is issued by a non-superuser, - indexes on shared catalogs will be skipped unless the user owns the - catalog (which typically won't be the case), has privileges of the - <literal>pg_maintain</literal> role, or has the <literal>MAINTAIN</literal> - 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. + other users. However, as a special exception, + <command>REINDEX DATABASE</command>, <command>REINDEX SCHEMA</command>, and + <command>REINDEX SYSTEM</command> will skip indexes on shared catalogs + unless the user has the <literal>MAINTAIN</literal> privilege on the + catalog. </para> <para> diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml index 57bc4c23ec..c42bbea9e2 100644 --- a/doc/src/sgml/ref/vacuum.sgml +++ b/doc/src/sgml/ref/vacuum.sgml @@ -445,17 +445,8 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet <para> To vacuum a table, one must ordinarily have the <literal>MAINTAIN</literal> - privilege on the table or be the table's owner, a superuser, or a role with - privileges of the - <link linkend="predefined-roles-table"><literal>pg_maintain</literal></link> - role. However, database owners are allowed to + privilege on the table. However, database owners are allowed to vacuum all tables in their databases, except shared catalogs. - (The restriction for shared catalogs means that a true database-wide - <command>VACUUM</command> can only be performed by superusers and roles - with privileges of <literal>pg_maintain</literal>.) If a role has - permission to <command>VACUUM</command> a partitioned table, it is also - permitted to <command>VACUUM</command> each of its partitions, regardless - of whether the role has the aforementioned privileges on the partition. <command>VACUUM</command> will skip over any tables that the calling user does not have permission to vacuum. </para> diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml index b6c37ccef2..e1540dd481 100644 --- a/doc/src/sgml/user-manag.sgml +++ b/doc/src/sgml/user-manag.sgml @@ -692,7 +692,8 @@ DROP ROLE doomed_role; <link linkend="sql-refreshmaterializedview"><command>REFRESH MATERIALIZED VIEW</command></link>, <link linkend="sql-reindex"><command>REINDEX</command></link>, and <link linkend="sql-lock"><command>LOCK TABLE</command></link> on all - relations.</entry> + relations, as if having <literal>MAINTAIN</literal> rights on those + objects, even without having it explicitly.</entry> </row> <row> <entry>pg_use_reserved_connections</entry> diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 369fea7c04..38834356b9 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -1693,11 +1693,8 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid) if (get_rel_relkind(indexrelid) != RELKIND_INDEX) continue; - /* - * 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; /* Use a permanent memory context for the result list */ old_context = MemoryContextSwitchTo(cluster_context); @@ -1720,8 +1717,7 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid) static bool cluster_is_permitted_for_relation(Oid relid, Oid userid) { - if (pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK || - has_partition_ancestor_privs(relid, userid, ACL_MAINTAIN)) + if (pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK) return true; ereport(WARNING, diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index a5168c9f09..ed28d13a16 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -2854,8 +2854,7 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation, /* Check permissions */ table_oid = IndexGetRelation(relId, true); if (OidIsValid(table_oid) && - pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && - !has_partition_ancestor_privs(table_oid, GetUserId(), ACL_MAINTAIN)) + pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK) aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_INDEX, relation->relname); @@ -3064,18 +3063,12 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, continue; /* - * The table can be reindexed if the user has been granted MAINTAIN on - * the table or one of its partition ancestors or the user is a - * superuser, the table owner, or the database/schema owner (but in - * the latter case, only if it's not a shared relation). - * pg_class_aclcheck includes the superuser case, and depending on - * objectKind we already know that the user has permission to run - * REINDEX on this database or schema per the permission checks at the - * beginning of this routine. + * We already checked privileges on the database or schema, but we + * further restrict reindexing shared catalogs to roles with the + * MAINTAIN privilege on the relation. */ if (classtuple->relisshared && - pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && - !has_partition_ancestor_privs(relid, GetUserId(), ACL_MAINTAIN)) + pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK) continue; /* diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c index 43c7d7f4bb..92662cbbc8 100644 --- a/src/backend/commands/lockcmds.c +++ b/src/backend/commands/lockcmds.c @@ -19,7 +19,6 @@ #include "catalog/namespace.h" #include "catalog/pg_inherits.h" #include "commands/lockcmds.h" -#include "commands/tablecmds.h" #include "miscadmin.h" #include "nodes/nodeFuncs.h" #include "parser/parse_clause.h" @@ -297,12 +296,5 @@ LockTableAclCheck(Oid reloid, LOCKMODE lockmode, Oid userid) aclresult = pg_class_aclcheck(reloid, userid, aclmask); - /* - * If this is a partition, check permissions of its ancestors if needed. - */ - if (aclresult != ACLCHECK_OK && - has_partition_ancestor_privs(reloid, userid, ACL_MAINTAIN)) - aclresult = ACLCHECK_OK; - return aclresult; } diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 4d49d70c33..29eece1c2c 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -17006,38 +17006,11 @@ RangeVarCallbackMaintainsTable(const RangeVar *relation, errmsg("\"%s\" is not a table or materialized view", relation->relname))); /* Check permissions */ - if (pg_class_aclcheck(relId, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && - !has_partition_ancestor_privs(relId, GetUserId(), ACL_MAINTAIN)) + if (pg_class_aclcheck(relId, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK) aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_TABLE, relation->relname); } -/* - * If relid is a partition, returns whether userid has any of the privileges - * specified in acl on any of its ancestors. Otherwise, returns false. - */ -bool -has_partition_ancestor_privs(Oid relid, Oid userid, AclMode acl) -{ - List *ancestors; - ListCell *lc; - - if (!get_rel_relispartition(relid)) - return false; - - ancestors = get_partition_ancestors(relid); - foreach(lc, ancestors) - { - Oid ancestor = lfirst_oid(lc); - - if (OidIsValid(ancestor) && - pg_class_aclcheck(ancestor, userid, acl) == ACLCHECK_OK) - return true; - } - - return false; -} - /* * Callback to RangeVarGetRelidExtended() for TRUNCATE processing. */ diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index a843f9ad92..2e41a31173 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -41,7 +41,6 @@ #include "catalog/pg_namespace.h" #include "commands/cluster.h" #include "commands/defrem.h" -#include "commands/tablecmds.h" #include "commands/vacuum.h" #include "miscadmin.h" #include "nodes/makefuncs.h" @@ -719,13 +718,10 @@ vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple, * - the role owns the relation * - the role owns the current database and the relation is not shared * - the role has been granted the MAINTAIN privilege on the relation - * - the role has privileges to vacuum/analyze any of the relation's - * partition ancestors *---------- */ if ((object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) && !reltuple->relisshared) || - pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) == ACLCHECK_OK || - has_partition_ancestor_privs(relid, GetUserId(), ACL_MAINTAIN)) + pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) == ACLCHECK_OK) return true; relname = NameStr(reltuple->relname); diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h index 17b9404937..250d89ff88 100644 --- a/src/include/commands/tablecmds.h +++ b/src/include/commands/tablecmds.h @@ -99,7 +99,6 @@ extern void AtEOSubXact_on_commit_actions(bool isCommit, extern void RangeVarCallbackMaintainsTable(const RangeVar *relation, Oid relId, Oid oldRelId, void *arg); -extern bool has_partition_ancestor_privs(Oid relid, Oid userid, AclMode acl); extern void RangeVarCallbackOwnsRelation(const RangeVar *relation, Oid relId, Oid oldRelId, void *arg); diff --git a/src/test/isolation/expected/cluster-conflict-partition.out b/src/test/isolation/expected/cluster-conflict-partition.out index 8d21276996..7be9e56ef1 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; <waiting ...> step s1_commit: COMMIT; step s2_cluster: <... completed> @@ -11,7 +11,7 @@ step s2_reset: RESET ROLE; starting permutation: s1_begin s2_auth s1_lock_parent s2_cluster s1_commit s2_reset step s1_begin: BEGIN; -step s2_auth: SET ROLE regress_cluster_part; +step s2_auth: SET ROLE regress_cluster_part; SET client_min_messages = ERROR; step s1_lock_parent: LOCK cluster_part_tab IN SHARE UPDATE EXCLUSIVE MODE; step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...> step s1_commit: COMMIT; @@ -21,17 +21,15 @@ step s2_reset: RESET ROLE; starting permutation: s1_begin s1_lock_child s2_auth s2_cluster s1_commit s2_reset step s1_begin: BEGIN; step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; -step s2_auth: SET ROLE regress_cluster_part; -step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...> +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> step s2_reset: RESET ROLE; starting permutation: s1_begin s2_auth s1_lock_child s2_cluster s1_commit s2_reset step s1_begin: BEGIN; -step s2_auth: SET ROLE regress_cluster_part; +step s2_auth: SET ROLE regress_cluster_part; SET client_min_messages = ERROR; step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; -step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...> +step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; step s1_commit: COMMIT; -step s2_cluster: <... completed> step s2_reset: RESET ROLE; diff --git a/src/test/isolation/specs/cluster-conflict-partition.spec b/src/test/isolation/specs/cluster-conflict-partition.spec index ae38cb4ee3..4d38a7f49a 100644 --- a/src/test/isolation/specs/cluster-conflict-partition.spec +++ b/src/test/isolation/specs/cluster-conflict-partition.spec @@ -23,12 +23,15 @@ step s1_lock_child { LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; step s1_commit { COMMIT; } session s2 -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 s2_reset { RESET ROLE; } -# CLUSTER waits if locked, passes for all cases. +# CLUSTER on the parent waits if locked, passes for all cases. permutation s1_begin s1_lock_parent s2_auth s2_cluster s1_commit s2_reset permutation s1_begin s2_auth s1_lock_parent s2_cluster s1_commit s2_reset + +# When taking a lock on a partition leaf, CLUSTER on the parent skips +# the leaf, passes for all cases. permutation s1_begin s1_lock_child s2_auth s2_cluster s1_commit s2_reset permutation s1_begin s2_auth s1_lock_child s2_cluster s1_commit s2_reset diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out index 2eec483eaa..27a5dff5d4 100644 --- a/src/test/regress/expected/cluster.out +++ b/src/test/regress/expected/cluster.out @@ -508,6 +508,7 @@ CREATE TEMP TABLE ptnowner_oldnodes AS JOIN pg_class AS c ON c.oid=tree.relid; SET SESSION AUTHORIZATION regress_ptnowner; CLUSTER ptnowner USING ptnowner_i_idx; +WARNING: permission denied to cluster "ptnowner2", skipping it RESET SESSION AUTHORIZATION; SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a JOIN ptnowner_oldnodes b USING (oid) ORDER BY a.relname COLLATE "C"; @@ -515,7 +516,7 @@ SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a -----------+---------- ptnowner | t ptnowner1 | f - ptnowner2 | f + ptnowner2 | t (3 rows) DROP TABLE ptnowner; diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out index 41e020cf20..4def90b805 100644 --- a/src/test/regress/expected/vacuum.out +++ b/src/test/regress/expected/vacuum.out @@ -442,14 +442,20 @@ ALTER TABLE vacowned_parted OWNER TO regress_vacuum; ALTER TABLE vacowned_part1 OWNER TO regress_vacuum; SET ROLE regress_vacuum; VACUUM vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM vacowned_part1; VACUUM vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it ANALYZE vacowned_parted; +WARNING: permission denied to analyze "vacowned_part2", skipping it ANALYZE vacowned_part1; ANALYZE vacowned_part2; +WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; VACUUM (ANALYZE) vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it RESET ROLE; -- Only one partition owned by other user. ALTER TABLE vacowned_parted OWNER TO CURRENT_USER; @@ -478,14 +484,26 @@ ALTER TABLE vacowned_parted OWNER TO regress_vacuum; ALTER TABLE vacowned_part1 OWNER TO CURRENT_USER; SET ROLE regress_vacuum; VACUUM vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part1", skipping it +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM vacowned_part1; +WARNING: permission denied to vacuum "vacowned_part1", skipping it VACUUM vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it ANALYZE vacowned_parted; +WARNING: permission denied to analyze "vacowned_part1", skipping it +WARNING: permission denied to analyze "vacowned_part2", skipping it ANALYZE vacowned_part1; +WARNING: permission denied to analyze "vacowned_part1", skipping it ANALYZE vacowned_part2; +WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part1", skipping it +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; +WARNING: permission denied to vacuum "vacowned_part1", skipping it VACUUM (ANALYZE) vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it RESET ROLE; DROP TABLE vacowned; DROP TABLE vacowned_parted; -- 2.25.1 --sdtB3X0nJg68CQEu Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v3-0002-convert-boolean-parameter-into-a-VACOPT_-option.patch" ^ permalink raw reply [nested|flat] 8+ messages in thread
* [PATCH v2 1/2] partial revert of ff9618e82a @ 2023-06-14 17:54 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Nathan Bossart @ 2023-06-14 17:54 UTC (permalink / raw) --- doc/src/sgml/ref/analyze.sgml | 13 ++------- doc/src/sgml/ref/cluster.sgml | 9 +----- doc/src/sgml/ref/lock.sgml | 24 ++++++--------- .../sgml/ref/refresh_materialized_view.sgml | 17 +++++------ doc/src/sgml/ref/reindex.sgml | 28 +++++++----------- doc/src/sgml/ref/vacuum.sgml | 13 ++------- doc/src/sgml/user-manag.sgml | 3 +- src/backend/commands/cluster.c | 10 ++----- src/backend/commands/indexcmds.c | 17 ++++------- src/backend/commands/lockcmds.c | 8 ----- src/backend/commands/tablecmds.c | 29 +------------------ src/backend/commands/vacuum.c | 6 +--- src/include/commands/tablecmds.h | 1 - .../expected/cluster-conflict-partition.out | 14 ++++----- .../specs/cluster-conflict-partition.spec | 7 +++-- src/test/regress/expected/cluster.out | 3 +- src/test/regress/expected/vacuum.out | 18 ++++++++++++ 17 files changed, 74 insertions(+), 146 deletions(-) diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml index 20c6f9939f..ecc7c884b4 100644 --- a/doc/src/sgml/ref/analyze.sgml +++ b/doc/src/sgml/ref/analyze.sgml @@ -183,17 +183,8 @@ ANALYZE [ VERBOSE ] [ <replaceable class="parameter">table_and_columns</replacea <para> To analyze a table, one must ordinarily have the <literal>MAINTAIN</literal> - privilege on the table or be the table's owner, a superuser, or a role with - privileges of the - <link linkend="predefined-roles-table"><literal>pg_maintain</literal></link> - role. However, database owners are allowed to - analyze all tables in their databases, except shared catalogs. - (The restriction for shared catalogs means that a true database-wide - <command>ANALYZE</command> can only be performed by superusers and roles - with privileges of <literal>pg_maintain</literal>.) If a role has - permission to <command>ANALYZE</command> a partitioned table, it is also - permitted to <command>ANALYZE</command> each of its partitions, regardless - of whether the role has the aforementioned privileges on the partition. + privilege on the table. However, database owners are allowed to analyze all + tables in their databases, except shared catalogs. <command>ANALYZE</command> will skip over any tables that the calling user does not have permission to analyze. </para> diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml index 29f0f1fd90..06f3d269e6 100644 --- a/doc/src/sgml/ref/cluster.sgml +++ b/doc/src/sgml/ref/cluster.sgml @@ -134,14 +134,7 @@ CLUSTER [VERBOSE] <para> To cluster a table, one must have the <literal>MAINTAIN</literal> privilege - on the table or be the table's owner, a superuser, or a role with - privileges of the - <link linkend="predefined-roles-table"><literal>pg_maintain</literal></link> - role. If a role has permission to <command>CLUSTER</command> a partitioned - table, it is also permitted to <command>CLUSTER</command> each of its - partitions, regardless of whether the role has the aforementioned - privileges on the partition. <command>CLUSTER</command> will skip over any - tables that the calling user does not have permission to cluster. + on the table. </para> <para> diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml index 5b3b2b793a..d22c6f8384 100644 --- a/doc/src/sgml/ref/lock.sgml +++ b/doc/src/sgml/ref/lock.sgml @@ -166,21 +166,15 @@ LOCK [ TABLE ] [ ONLY ] <replaceable class="parameter">name</replaceable> [ * ] <para> To lock a table, the user must have the right privilege for the specified - <replaceable class="parameter">lockmode</replaceable>, or be the table's - owner, a superuser, or a role with privileges of the <link - linkend="predefined-roles-table"><literal>pg_maintain</literal></link> - role. If the user has <literal>MAINTAIN</literal>, - <literal>UPDATE</literal>, <literal>DELETE</literal>, or - <literal>TRUNCATE</literal> privileges on the table, any <replaceable - class="parameter">lockmode</replaceable> is permitted. If the user has - <literal>INSERT</literal> privileges on the table, <literal>ROW EXCLUSIVE - MODE</literal> (or a less-conflicting mode as described in <xref - linkend="explicit-locking"/>) is permitted. If a user has - <literal>SELECT</literal> privileges on the table, <literal>ACCESS SHARE - MODE</literal> is permitted. If a role has permission to lock a - partitioned table, it is also permitted to lock each of its partitions, - regardless of whether the role has the aforementioned privileges on the - partition. + <replaceable class="parameter">lockmode</replaceable>. If the user has + <literal>MAINTAIN</literal>, <literal>UPDATE</literal>, + <literal>DELETE</literal>, or <literal>TRUNCATE</literal> privileges on the + table, any <replaceable class="parameter">lockmode</replaceable> is + permitted. If the user has <literal>INSERT</literal> privileges on the + table, <literal>ROW EXCLUSIVE MODE</literal> (or a less-conflicting mode as + described in <xref linkend="explicit-locking"/>) is permitted. If a user + has <literal>SELECT</literal> privileges on the table, + <literal>ACCESS SHARE MODE</literal> is permitted. </para> <para> diff --git a/doc/src/sgml/ref/refresh_materialized_view.sgml b/doc/src/sgml/ref/refresh_materialized_view.sgml index 4d79b6ae7f..199a577d36 100644 --- a/doc/src/sgml/ref/refresh_materialized_view.sgml +++ b/doc/src/sgml/ref/refresh_materialized_view.sgml @@ -31,16 +31,13 @@ REFRESH MATERIALIZED VIEW [ CONCURRENTLY ] <replaceable class="parameter">name</ <para> <command>REFRESH MATERIALIZED VIEW</command> completely replaces the - contents of a materialized view. To execute this command you must be the - owner of the materialized view, have privileges of the - <link linkend="predefined-roles-table"><literal>pg_maintain</literal></link> - role, or have the <literal>MAINTAIN</literal> - privilege on the materialized view. The old contents are discarded. If - <literal>WITH DATA</literal> is specified (or defaults) the backing query - is executed to provide the new data, and the materialized view is left in a - scannable state. If <literal>WITH NO DATA</literal> is specified no new - data is generated and the materialized view is left in an unscannable - state. + contents of a materialized view. To execute this command you must have the + <literal>MAINTAIN</literal> privilege on the materialized view. The old + contents are discarded. If <literal>WITH DATA</literal> is specified (or + defaults) the backing query is executed to provide the new data, and the + materialized view is left in a scannable state. If + <literal>WITH NO DATA</literal> is specified no new data is generated and + the materialized view is left in an unscannable state. </para> <para> <literal>CONCURRENTLY</literal> and <literal>WITH NO DATA</literal> may not diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml index 71455dfdc7..a64b021e66 100644 --- a/doc/src/sgml/ref/reindex.sgml +++ b/doc/src/sgml/ref/reindex.sgml @@ -292,25 +292,17 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { DA </para> <para> - Reindexing a single index or table requires being the owner of that - index or table, having privileges of the + Reindexing a single index or table requires having the + <literal>MAINTAIN</literal> privilege on the table. Reindexing a schema or + database requires being the owner of that schema or database or having + privileges of the <link linkend="predefined-roles-table"><literal>pg_maintain</literal></link> - role, or having the <literal>MAINTAIN</literal> privilege on the - table. Reindexing a schema or database requires being the - owner of that schema or database or having privileges of the - <literal>pg_maintain</literal> role. Note specifically that it's thus - possible for non-superusers to rebuild indexes of tables owned by - other users. However, as a special exception, when - <command>REINDEX DATABASE</command>, <command>REINDEX SCHEMA</command> - or <command>REINDEX SYSTEM</command> is issued by a non-superuser, - indexes on shared catalogs will be skipped unless the user owns the - catalog (which typically won't be the case), has privileges of the - <literal>pg_maintain</literal> role, or has the <literal>MAINTAIN</literal> - 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. + role. Note specifically that it's thus possible for non-superusers to + rebuild indexes of tables owned by other users. However, as a special + exception, <command>REINDEX DATABASE</command>, + <command>REINDEX SCHEMA</command>, and <command>REINDEX SYSTEM</command> + will skip indexes on shared catalogs unless the user has the + <literal>MAINTAIN</literal> privilege on the catalog. </para> <para> diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml index 57bc4c23ec..fa2ee76e25 100644 --- a/doc/src/sgml/ref/vacuum.sgml +++ b/doc/src/sgml/ref/vacuum.sgml @@ -445,17 +445,8 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet <para> To vacuum a table, one must ordinarily have the <literal>MAINTAIN</literal> - privilege on the table or be the table's owner, a superuser, or a role with - privileges of the - <link linkend="predefined-roles-table"><literal>pg_maintain</literal></link> - role. However, database owners are allowed to - vacuum all tables in their databases, except shared catalogs. - (The restriction for shared catalogs means that a true database-wide - <command>VACUUM</command> can only be performed by superusers and roles - with privileges of <literal>pg_maintain</literal>.) If a role has - permission to <command>VACUUM</command> a partitioned table, it is also - permitted to <command>VACUUM</command> each of its partitions, regardless - of whether the role has the aforementioned privileges on the partition. + privilege on the table. However, database owners are allowed to vacuum all + tables in their databases, except shared catalogs. <command>VACUUM</command> will skip over any tables that the calling user does not have permission to vacuum. </para> diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml index b6c37ccef2..e1540dd481 100644 --- a/doc/src/sgml/user-manag.sgml +++ b/doc/src/sgml/user-manag.sgml @@ -692,7 +692,8 @@ DROP ROLE doomed_role; <link linkend="sql-refreshmaterializedview"><command>REFRESH MATERIALIZED VIEW</command></link>, <link linkend="sql-reindex"><command>REINDEX</command></link>, and <link linkend="sql-lock"><command>LOCK TABLE</command></link> on all - relations.</entry> + relations, as if having <literal>MAINTAIN</literal> rights on those + objects, even without having it explicitly.</entry> </row> <row> <entry>pg_use_reserved_connections</entry> diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 369fea7c04..38834356b9 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -1693,11 +1693,8 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid) if (get_rel_relkind(indexrelid) != RELKIND_INDEX) continue; - /* - * 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; /* Use a permanent memory context for the result list */ old_context = MemoryContextSwitchTo(cluster_context); @@ -1720,8 +1717,7 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid) static bool cluster_is_permitted_for_relation(Oid relid, Oid userid) { - if (pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK || - has_partition_ancestor_privs(relid, userid, ACL_MAINTAIN)) + if (pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK) return true; ereport(WARNING, diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index a5168c9f09..ed28d13a16 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -2854,8 +2854,7 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation, /* Check permissions */ table_oid = IndexGetRelation(relId, true); if (OidIsValid(table_oid) && - pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && - !has_partition_ancestor_privs(table_oid, GetUserId(), ACL_MAINTAIN)) + pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK) aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_INDEX, relation->relname); @@ -3064,18 +3063,12 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, continue; /* - * The table can be reindexed if the user has been granted MAINTAIN on - * the table or one of its partition ancestors or the user is a - * superuser, the table owner, or the database/schema owner (but in - * the latter case, only if it's not a shared relation). - * pg_class_aclcheck includes the superuser case, and depending on - * objectKind we already know that the user has permission to run - * REINDEX on this database or schema per the permission checks at the - * beginning of this routine. + * We already checked privileges on the database or schema, but we + * further restrict reindexing shared catalogs to roles with the + * MAINTAIN privilege on the relation. */ if (classtuple->relisshared && - pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && - !has_partition_ancestor_privs(relid, GetUserId(), ACL_MAINTAIN)) + pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK) continue; /* diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c index 43c7d7f4bb..92662cbbc8 100644 --- a/src/backend/commands/lockcmds.c +++ b/src/backend/commands/lockcmds.c @@ -19,7 +19,6 @@ #include "catalog/namespace.h" #include "catalog/pg_inherits.h" #include "commands/lockcmds.h" -#include "commands/tablecmds.h" #include "miscadmin.h" #include "nodes/nodeFuncs.h" #include "parser/parse_clause.h" @@ -297,12 +296,5 @@ LockTableAclCheck(Oid reloid, LOCKMODE lockmode, Oid userid) aclresult = pg_class_aclcheck(reloid, userid, aclmask); - /* - * If this is a partition, check permissions of its ancestors if needed. - */ - if (aclresult != ACLCHECK_OK && - has_partition_ancestor_privs(reloid, userid, ACL_MAINTAIN)) - aclresult = ACLCHECK_OK; - return aclresult; } diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 4d49d70c33..29eece1c2c 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -17006,38 +17006,11 @@ RangeVarCallbackMaintainsTable(const RangeVar *relation, errmsg("\"%s\" is not a table or materialized view", relation->relname))); /* Check permissions */ - if (pg_class_aclcheck(relId, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && - !has_partition_ancestor_privs(relId, GetUserId(), ACL_MAINTAIN)) + if (pg_class_aclcheck(relId, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK) aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_TABLE, relation->relname); } -/* - * If relid is a partition, returns whether userid has any of the privileges - * specified in acl on any of its ancestors. Otherwise, returns false. - */ -bool -has_partition_ancestor_privs(Oid relid, Oid userid, AclMode acl) -{ - List *ancestors; - ListCell *lc; - - if (!get_rel_relispartition(relid)) - return false; - - ancestors = get_partition_ancestors(relid); - foreach(lc, ancestors) - { - Oid ancestor = lfirst_oid(lc); - - if (OidIsValid(ancestor) && - pg_class_aclcheck(ancestor, userid, acl) == ACLCHECK_OK) - return true; - } - - return false; -} - /* * Callback to RangeVarGetRelidExtended() for TRUNCATE processing. */ diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index a843f9ad92..2e41a31173 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -41,7 +41,6 @@ #include "catalog/pg_namespace.h" #include "commands/cluster.h" #include "commands/defrem.h" -#include "commands/tablecmds.h" #include "commands/vacuum.h" #include "miscadmin.h" #include "nodes/makefuncs.h" @@ -719,13 +718,10 @@ vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple, * - the role owns the relation * - the role owns the current database and the relation is not shared * - the role has been granted the MAINTAIN privilege on the relation - * - the role has privileges to vacuum/analyze any of the relation's - * partition ancestors *---------- */ if ((object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) && !reltuple->relisshared) || - pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) == ACLCHECK_OK || - has_partition_ancestor_privs(relid, GetUserId(), ACL_MAINTAIN)) + pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) == ACLCHECK_OK) return true; relname = NameStr(reltuple->relname); diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h index 17b9404937..250d89ff88 100644 --- a/src/include/commands/tablecmds.h +++ b/src/include/commands/tablecmds.h @@ -99,7 +99,6 @@ extern void AtEOSubXact_on_commit_actions(bool isCommit, extern void RangeVarCallbackMaintainsTable(const RangeVar *relation, Oid relId, Oid oldRelId, void *arg); -extern bool has_partition_ancestor_privs(Oid relid, Oid userid, AclMode acl); extern void RangeVarCallbackOwnsRelation(const RangeVar *relation, Oid relId, Oid oldRelId, void *arg); diff --git a/src/test/isolation/expected/cluster-conflict-partition.out b/src/test/isolation/expected/cluster-conflict-partition.out index 8d21276996..7be9e56ef1 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; <waiting ...> step s1_commit: COMMIT; step s2_cluster: <... completed> @@ -11,7 +11,7 @@ step s2_reset: RESET ROLE; starting permutation: s1_begin s2_auth s1_lock_parent s2_cluster s1_commit s2_reset step s1_begin: BEGIN; -step s2_auth: SET ROLE regress_cluster_part; +step s2_auth: SET ROLE regress_cluster_part; SET client_min_messages = ERROR; step s1_lock_parent: LOCK cluster_part_tab IN SHARE UPDATE EXCLUSIVE MODE; step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...> step s1_commit: COMMIT; @@ -21,17 +21,15 @@ step s2_reset: RESET ROLE; starting permutation: s1_begin s1_lock_child s2_auth s2_cluster s1_commit s2_reset step s1_begin: BEGIN; step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; -step s2_auth: SET ROLE regress_cluster_part; -step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...> +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> step s2_reset: RESET ROLE; starting permutation: s1_begin s2_auth s1_lock_child s2_cluster s1_commit s2_reset step s1_begin: BEGIN; -step s2_auth: SET ROLE regress_cluster_part; +step s2_auth: SET ROLE regress_cluster_part; SET client_min_messages = ERROR; step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; -step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...> +step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; step s1_commit: COMMIT; -step s2_cluster: <... completed> step s2_reset: RESET ROLE; diff --git a/src/test/isolation/specs/cluster-conflict-partition.spec b/src/test/isolation/specs/cluster-conflict-partition.spec index ae38cb4ee3..4d38a7f49a 100644 --- a/src/test/isolation/specs/cluster-conflict-partition.spec +++ b/src/test/isolation/specs/cluster-conflict-partition.spec @@ -23,12 +23,15 @@ step s1_lock_child { LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; step s1_commit { COMMIT; } session s2 -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 s2_reset { RESET ROLE; } -# CLUSTER waits if locked, passes for all cases. +# CLUSTER on the parent waits if locked, passes for all cases. permutation s1_begin s1_lock_parent s2_auth s2_cluster s1_commit s2_reset permutation s1_begin s2_auth s1_lock_parent s2_cluster s1_commit s2_reset + +# When taking a lock on a partition leaf, CLUSTER on the parent skips +# the leaf, passes for all cases. permutation s1_begin s1_lock_child s2_auth s2_cluster s1_commit s2_reset permutation s1_begin s2_auth s1_lock_child s2_cluster s1_commit s2_reset diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out index 2eec483eaa..27a5dff5d4 100644 --- a/src/test/regress/expected/cluster.out +++ b/src/test/regress/expected/cluster.out @@ -508,6 +508,7 @@ CREATE TEMP TABLE ptnowner_oldnodes AS JOIN pg_class AS c ON c.oid=tree.relid; SET SESSION AUTHORIZATION regress_ptnowner; CLUSTER ptnowner USING ptnowner_i_idx; +WARNING: permission denied to cluster "ptnowner2", skipping it RESET SESSION AUTHORIZATION; SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a JOIN ptnowner_oldnodes b USING (oid) ORDER BY a.relname COLLATE "C"; @@ -515,7 +516,7 @@ SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a -----------+---------- ptnowner | t ptnowner1 | f - ptnowner2 | f + ptnowner2 | t (3 rows) DROP TABLE ptnowner; diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out index 41e020cf20..4def90b805 100644 --- a/src/test/regress/expected/vacuum.out +++ b/src/test/regress/expected/vacuum.out @@ -442,14 +442,20 @@ ALTER TABLE vacowned_parted OWNER TO regress_vacuum; ALTER TABLE vacowned_part1 OWNER TO regress_vacuum; SET ROLE regress_vacuum; VACUUM vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM vacowned_part1; VACUUM vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it ANALYZE vacowned_parted; +WARNING: permission denied to analyze "vacowned_part2", skipping it ANALYZE vacowned_part1; ANALYZE vacowned_part2; +WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; VACUUM (ANALYZE) vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it RESET ROLE; -- Only one partition owned by other user. ALTER TABLE vacowned_parted OWNER TO CURRENT_USER; @@ -478,14 +484,26 @@ ALTER TABLE vacowned_parted OWNER TO regress_vacuum; ALTER TABLE vacowned_part1 OWNER TO CURRENT_USER; SET ROLE regress_vacuum; VACUUM vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part1", skipping it +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM vacowned_part1; +WARNING: permission denied to vacuum "vacowned_part1", skipping it VACUUM vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it ANALYZE vacowned_parted; +WARNING: permission denied to analyze "vacowned_part1", skipping it +WARNING: permission denied to analyze "vacowned_part2", skipping it ANALYZE vacowned_part1; +WARNING: permission denied to analyze "vacowned_part1", skipping it ANALYZE vacowned_part2; +WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part1", skipping it +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; +WARNING: permission denied to vacuum "vacowned_part1", skipping it VACUUM (ANALYZE) vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it RESET ROLE; DROP TABLE vacowned; DROP TABLE vacowned_parted; -- 2.25.1 --EVF5PPMfhYS0aIcm Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v2-0002-convert-boolean-parameter-into-a-VACOPT_-option.patch" ^ permalink raw reply [nested|flat] 8+ messages in thread
* [PATCH v4 2/3] partial revert of ff9618e82a @ 2023-06-19 20:57 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Nathan Bossart @ 2023-06-19 20:57 UTC (permalink / raw) --- doc/src/sgml/ref/analyze.sgml | 5 +-- doc/src/sgml/ref/cluster.sgml | 5 +-- doc/src/sgml/ref/lock.sgml | 5 +-- doc/src/sgml/ref/reindex.sgml | 6 +--- doc/src/sgml/ref/vacuum.sgml | 5 +-- src/backend/commands/cluster.c | 10 ++---- src/backend/commands/indexcmds.c | 27 +++++++-------- src/backend/commands/lockcmds.c | 8 ----- src/backend/commands/tablecmds.c | 34 +++---------------- src/backend/commands/vacuum.c | 8 +---- src/include/commands/tablecmds.h | 1 - .../expected/cluster-conflict-partition.out | 14 ++++---- .../specs/cluster-conflict-partition.spec | 7 ++-- src/test/regress/expected/cluster.out | 3 +- src/test/regress/expected/create_index.out | 4 +-- src/test/regress/expected/privileges.out | 8 ++--- src/test/regress/expected/vacuum.out | 18 ++++++++++ 17 files changed, 62 insertions(+), 106 deletions(-) diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml index 20c6f9939f..30a893230e 100644 --- a/doc/src/sgml/ref/analyze.sgml +++ b/doc/src/sgml/ref/analyze.sgml @@ -190,10 +190,7 @@ ANALYZE [ VERBOSE ] [ <replaceable class="parameter">table_and_columns</replacea analyze all tables in their databases, except shared catalogs. (The restriction for shared catalogs means that a true database-wide <command>ANALYZE</command> can only be performed by superusers and roles - with privileges of <literal>pg_maintain</literal>.) If a role has - permission to <command>ANALYZE</command> a partitioned table, it is also - permitted to <command>ANALYZE</command> each of its partitions, regardless - of whether the role has the aforementioned privileges on the partition. + with privileges of <literal>pg_maintain</literal>.) <command>ANALYZE</command> will skip over any tables that the calling user does not have permission to analyze. </para> diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml index 29f0f1fd90..f0dd7faed5 100644 --- a/doc/src/sgml/ref/cluster.sgml +++ b/doc/src/sgml/ref/cluster.sgml @@ -137,10 +137,7 @@ CLUSTER [VERBOSE] on the table or be the table's owner, a superuser, or a role with privileges of the <link linkend="predefined-roles-table"><literal>pg_maintain</literal></link> - role. If a role has permission to <command>CLUSTER</command> a partitioned - table, it is also permitted to <command>CLUSTER</command> each of its - partitions, regardless of whether the role has the aforementioned - privileges on the partition. <command>CLUSTER</command> will skip over any + role. <command>CLUSTER</command> will skip over any tables that the calling user does not have permission to cluster. </para> diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml index 5b3b2b793a..8524182211 100644 --- a/doc/src/sgml/ref/lock.sgml +++ b/doc/src/sgml/ref/lock.sgml @@ -177,10 +177,7 @@ LOCK [ TABLE ] [ ONLY ] <replaceable class="parameter">name</replaceable> [ * ] MODE</literal> (or a less-conflicting mode as described in <xref linkend="explicit-locking"/>) is permitted. If a user has <literal>SELECT</literal> privileges on the table, <literal>ACCESS SHARE - MODE</literal> is permitted. If a role has permission to lock a - partitioned table, it is also permitted to lock each of its partitions, - regardless of whether the role has the aforementioned privileges on the - partition. + MODE</literal> is permitted. </para> <para> diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml index 71455dfdc7..23f8c7630b 100644 --- a/doc/src/sgml/ref/reindex.sgml +++ b/doc/src/sgml/ref/reindex.sgml @@ -306,11 +306,7 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { DA indexes on shared catalogs will be skipped unless the user owns the catalog (which typically won't be the case), has privileges of the <literal>pg_maintain</literal> role, or has the <literal>MAINTAIN</literal> - 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. </para> <para> diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml index 57bc4c23ec..445325e14c 100644 --- a/doc/src/sgml/ref/vacuum.sgml +++ b/doc/src/sgml/ref/vacuum.sgml @@ -452,10 +452,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet vacuum all tables in their databases, except shared catalogs. (The restriction for shared catalogs means that a true database-wide <command>VACUUM</command> can only be performed by superusers and roles - with privileges of <literal>pg_maintain</literal>.) If a role has - permission to <command>VACUUM</command> a partitioned table, it is also - permitted to <command>VACUUM</command> each of its partitions, regardless - of whether the role has the aforementioned privileges on the partition. + with privileges of <literal>pg_maintain</literal>.) <command>VACUUM</command> will skip over any tables that the calling user does not have permission to vacuum. </para> diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 369fea7c04..38834356b9 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -1693,11 +1693,8 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid) if (get_rel_relkind(indexrelid) != RELKIND_INDEX) continue; - /* - * 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; /* Use a permanent memory context for the result list */ old_context = MemoryContextSwitchTo(cluster_context); @@ -1720,8 +1717,7 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid) static bool cluster_is_permitted_for_relation(Oid relid, Oid userid) { - if (pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK || - has_partition_ancestor_privs(relid, userid, ACL_MAINTAIN)) + if (pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK) return true; ereport(WARNING, diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index a5168c9f09..9bc97e1fc2 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -2853,11 +2853,14 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation, /* Check permissions */ table_oid = IndexGetRelation(relId, true); - if (OidIsValid(table_oid) && - pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && - !has_partition_ancestor_privs(table_oid, GetUserId(), ACL_MAINTAIN)) - aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_INDEX, - relation->relname); + if (OidIsValid(table_oid)) + { + AclResult aclresult; + + aclresult = pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN); + if (aclresult != ACLCHECK_OK) + aclcheck_error(aclresult, OBJECT_INDEX, relation->relname); + } /* Lock heap before index to avoid deadlock. */ if (relId != oldRelId) @@ -3064,18 +3067,12 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, continue; /* - * The table can be reindexed if the user has been granted MAINTAIN on - * the table or one of its partition ancestors or the user is a - * superuser, the table owner, or the database/schema owner (but in - * the latter case, only if it's not a shared relation). - * pg_class_aclcheck includes the superuser case, and depending on - * objectKind we already know that the user has permission to run - * REINDEX on this database or schema per the permission checks at the - * beginning of this routine. + * We already checked privileges on the database or schema, but we + * further restrict reindexing shared catalogs to roles with the + * MAINTAIN privilege on the relation. */ if (classtuple->relisshared && - pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && - !has_partition_ancestor_privs(relid, GetUserId(), ACL_MAINTAIN)) + pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK) continue; /* diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c index 43c7d7f4bb..92662cbbc8 100644 --- a/src/backend/commands/lockcmds.c +++ b/src/backend/commands/lockcmds.c @@ -19,7 +19,6 @@ #include "catalog/namespace.h" #include "catalog/pg_inherits.h" #include "commands/lockcmds.h" -#include "commands/tablecmds.h" #include "miscadmin.h" #include "nodes/nodeFuncs.h" #include "parser/parse_clause.h" @@ -297,12 +296,5 @@ LockTableAclCheck(Oid reloid, LOCKMODE lockmode, Oid userid) aclresult = pg_class_aclcheck(reloid, userid, aclmask); - /* - * If this is a partition, check permissions of its ancestors if needed. - */ - if (aclresult != ACLCHECK_OK && - has_partition_ancestor_privs(reloid, userid, ACL_MAINTAIN)) - aclresult = ACLCHECK_OK; - return aclresult; } diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 4d49d70c33..9b12bc44d7 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -16986,6 +16986,7 @@ RangeVarCallbackMaintainsTable(const RangeVar *relation, Oid relId, Oid oldRelId, void *arg) { char relkind; + AclResult aclresult; /* Nothing to do if the relation was not found. */ if (!OidIsValid(relId)) @@ -17006,36 +17007,9 @@ RangeVarCallbackMaintainsTable(const RangeVar *relation, errmsg("\"%s\" is not a table or materialized view", relation->relname))); /* Check permissions */ - if (pg_class_aclcheck(relId, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && - !has_partition_ancestor_privs(relId, GetUserId(), ACL_MAINTAIN)) - aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_TABLE, - relation->relname); -} - -/* - * If relid is a partition, returns whether userid has any of the privileges - * specified in acl on any of its ancestors. Otherwise, returns false. - */ -bool -has_partition_ancestor_privs(Oid relid, Oid userid, AclMode acl) -{ - List *ancestors; - ListCell *lc; - - if (!get_rel_relispartition(relid)) - return false; - - ancestors = get_partition_ancestors(relid); - foreach(lc, ancestors) - { - Oid ancestor = lfirst_oid(lc); - - if (OidIsValid(ancestor) && - pg_class_aclcheck(ancestor, userid, acl) == ACLCHECK_OK) - return true; - } - - return false; + aclresult = pg_class_aclcheck(relId, GetUserId(), ACL_MAINTAIN); + if (aclresult != ACLCHECK_OK) + aclcheck_error(aclresult, OBJECT_TABLE, relation->relname); } /* diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 987b11d16c..11c23de8c1 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -41,7 +41,6 @@ #include "catalog/pg_namespace.h" #include "commands/cluster.h" #include "commands/defrem.h" -#include "commands/tablecmds.h" #include "commands/vacuum.h" #include "miscadmin.h" #include "nodes/makefuncs.h" @@ -721,17 +720,12 @@ vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple, /*---------- * A role has privileges to vacuum or analyze the relation if any of the * following are true: - * - the role is a superuser - * - the role owns the relation * - the role owns the current database and the relation is not shared * - the role has been granted the MAINTAIN privilege on the relation - * - the role has privileges to vacuum/analyze any of the relation's - * partition ancestors *---------- */ if ((object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) && !reltuple->relisshared) || - pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) == ACLCHECK_OK || - has_partition_ancestor_privs(relid, GetUserId(), ACL_MAINTAIN)) + pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) == ACLCHECK_OK) return true; relname = NameStr(reltuple->relname); diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h index 17b9404937..250d89ff88 100644 --- a/src/include/commands/tablecmds.h +++ b/src/include/commands/tablecmds.h @@ -99,7 +99,6 @@ extern void AtEOSubXact_on_commit_actions(bool isCommit, extern void RangeVarCallbackMaintainsTable(const RangeVar *relation, Oid relId, Oid oldRelId, void *arg); -extern bool has_partition_ancestor_privs(Oid relid, Oid userid, AclMode acl); extern void RangeVarCallbackOwnsRelation(const RangeVar *relation, Oid relId, Oid oldRelId, void *arg); diff --git a/src/test/isolation/expected/cluster-conflict-partition.out b/src/test/isolation/expected/cluster-conflict-partition.out index 8d21276996..7be9e56ef1 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; <waiting ...> step s1_commit: COMMIT; step s2_cluster: <... completed> @@ -11,7 +11,7 @@ step s2_reset: RESET ROLE; starting permutation: s1_begin s2_auth s1_lock_parent s2_cluster s1_commit s2_reset step s1_begin: BEGIN; -step s2_auth: SET ROLE regress_cluster_part; +step s2_auth: SET ROLE regress_cluster_part; SET client_min_messages = ERROR; step s1_lock_parent: LOCK cluster_part_tab IN SHARE UPDATE EXCLUSIVE MODE; step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...> step s1_commit: COMMIT; @@ -21,17 +21,15 @@ step s2_reset: RESET ROLE; starting permutation: s1_begin s1_lock_child s2_auth s2_cluster s1_commit s2_reset step s1_begin: BEGIN; step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; -step s2_auth: SET ROLE regress_cluster_part; -step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...> +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> step s2_reset: RESET ROLE; starting permutation: s1_begin s2_auth s1_lock_child s2_cluster s1_commit s2_reset step s1_begin: BEGIN; -step s2_auth: SET ROLE regress_cluster_part; +step s2_auth: SET ROLE regress_cluster_part; SET client_min_messages = ERROR; step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; -step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...> +step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; step s1_commit: COMMIT; -step s2_cluster: <... completed> step s2_reset: RESET ROLE; diff --git a/src/test/isolation/specs/cluster-conflict-partition.spec b/src/test/isolation/specs/cluster-conflict-partition.spec index ae38cb4ee3..4d38a7f49a 100644 --- a/src/test/isolation/specs/cluster-conflict-partition.spec +++ b/src/test/isolation/specs/cluster-conflict-partition.spec @@ -23,12 +23,15 @@ step s1_lock_child { LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; step s1_commit { COMMIT; } session s2 -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 s2_reset { RESET ROLE; } -# CLUSTER waits if locked, passes for all cases. +# CLUSTER on the parent waits if locked, passes for all cases. permutation s1_begin s1_lock_parent s2_auth s2_cluster s1_commit s2_reset permutation s1_begin s2_auth s1_lock_parent s2_cluster s1_commit s2_reset + +# When taking a lock on a partition leaf, CLUSTER on the parent skips +# the leaf, passes for all cases. permutation s1_begin s1_lock_child s2_auth s2_cluster s1_commit s2_reset permutation s1_begin s2_auth s1_lock_child s2_cluster s1_commit s2_reset diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out index 2eec483eaa..27a5dff5d4 100644 --- a/src/test/regress/expected/cluster.out +++ b/src/test/regress/expected/cluster.out @@ -508,6 +508,7 @@ CREATE TEMP TABLE ptnowner_oldnodes AS JOIN pg_class AS c ON c.oid=tree.relid; SET SESSION AUTHORIZATION regress_ptnowner; CLUSTER ptnowner USING ptnowner_i_idx; +WARNING: permission denied to cluster "ptnowner2", skipping it RESET SESSION AUTHORIZATION; SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a JOIN ptnowner_oldnodes b USING (oid) ORDER BY a.relname COLLATE "C"; @@ -515,7 +516,7 @@ SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a -----------+---------- ptnowner | t ptnowner1 | f - ptnowner2 | f + ptnowner2 | t (3 rows) DROP TABLE ptnowner; diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out index acfd9d1f4f..1473bc3175 100644 --- a/src/test/regress/expected/create_index.out +++ b/src/test/regress/expected/create_index.out @@ -2831,9 +2831,9 @@ RESET ROLE; GRANT USAGE ON SCHEMA pg_toast TO regress_reindexuser; SET SESSION ROLE regress_reindexuser; REINDEX TABLE pg_toast.pg_toast_1260; -ERROR: must be owner of table pg_toast_1260 +ERROR: permission denied for table pg_toast_1260 REINDEX INDEX pg_toast.pg_toast_1260_index; -ERROR: must be owner of index pg_toast_1260_index +ERROR: permission denied for index pg_toast_1260_index -- Clean up RESET ROLE; REVOKE USAGE ON SCHEMA pg_toast FROM regress_reindexuser; diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out index 3cf4ac8c9e..3e4dfcc2ec 100644 --- a/src/test/regress/expected/privileges.out +++ b/src/test/regress/expected/privileges.out @@ -2928,13 +2928,13 @@ WARNING: permission denied to analyze "maintain_test", skipping it VACUUM (ANALYZE) maintain_test; WARNING: permission denied to vacuum "maintain_test", skipping it CLUSTER maintain_test USING maintain_test_a_idx; -ERROR: must be owner of table maintain_test +ERROR: permission denied for table maintain_test REFRESH MATERIALIZED VIEW refresh_test; -ERROR: must be owner of table refresh_test +ERROR: permission denied for table refresh_test REINDEX TABLE maintain_test; -ERROR: must be owner of table maintain_test +ERROR: permission denied for table maintain_test REINDEX INDEX maintain_test_a_idx; -ERROR: must be owner of index maintain_test_a_idx +ERROR: permission denied for index maintain_test_a_idx REINDEX SCHEMA reindex_test; ERROR: must be owner of schema reindex_test RESET ROLE; diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out index 41e020cf20..4def90b805 100644 --- a/src/test/regress/expected/vacuum.out +++ b/src/test/regress/expected/vacuum.out @@ -442,14 +442,20 @@ ALTER TABLE vacowned_parted OWNER TO regress_vacuum; ALTER TABLE vacowned_part1 OWNER TO regress_vacuum; SET ROLE regress_vacuum; VACUUM vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM vacowned_part1; VACUUM vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it ANALYZE vacowned_parted; +WARNING: permission denied to analyze "vacowned_part2", skipping it ANALYZE vacowned_part1; ANALYZE vacowned_part2; +WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; VACUUM (ANALYZE) vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it RESET ROLE; -- Only one partition owned by other user. ALTER TABLE vacowned_parted OWNER TO CURRENT_USER; @@ -478,14 +484,26 @@ ALTER TABLE vacowned_parted OWNER TO regress_vacuum; ALTER TABLE vacowned_part1 OWNER TO CURRENT_USER; SET ROLE regress_vacuum; VACUUM vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part1", skipping it +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM vacowned_part1; +WARNING: permission denied to vacuum "vacowned_part1", skipping it VACUUM vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it ANALYZE vacowned_parted; +WARNING: permission denied to analyze "vacowned_part1", skipping it +WARNING: permission denied to analyze "vacowned_part2", skipping it ANALYZE vacowned_part1; +WARNING: permission denied to analyze "vacowned_part1", skipping it ANALYZE vacowned_part2; +WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part1", skipping it +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; +WARNING: permission denied to vacuum "vacowned_part1", skipping it VACUUM (ANALYZE) vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it RESET ROLE; DROP TABLE vacowned; DROP TABLE vacowned_parted; -- 2.25.1 --/9DWx/yDrRhgMJTb Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v4-0003-simplify-privilege-related-documentation-for-main.patch" ^ permalink raw reply [nested|flat] 8+ messages in thread
* [PATCH v7 1/2] partial revert of ff9618e82a @ 2023-06-19 20:57 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Nathan Bossart @ 2023-06-19 20:57 UTC (permalink / raw) --- doc/src/sgml/ref/analyze.sgml | 5 +-- doc/src/sgml/ref/cluster.sgml | 5 +-- doc/src/sgml/ref/lock.sgml | 5 +-- doc/src/sgml/ref/reindex.sgml | 6 +--- doc/src/sgml/ref/vacuum.sgml | 5 +-- src/backend/commands/cluster.c | 10 ++---- src/backend/commands/indexcmds.c | 27 +++++++-------- src/backend/commands/lockcmds.c | 8 ----- src/backend/commands/tablecmds.c | 34 +++---------------- src/backend/commands/vacuum.c | 8 +---- src/include/commands/tablecmds.h | 1 - .../expected/cluster-conflict-partition.out | 14 ++++---- .../specs/cluster-conflict-partition.spec | 7 ++-- src/test/regress/expected/cluster.out | 3 +- src/test/regress/expected/create_index.out | 4 +-- src/test/regress/expected/privileges.out | 8 ++--- src/test/regress/expected/vacuum.out | 18 ++++++++++ 17 files changed, 62 insertions(+), 106 deletions(-) diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml index 20c6f9939f..30a893230e 100644 --- a/doc/src/sgml/ref/analyze.sgml +++ b/doc/src/sgml/ref/analyze.sgml @@ -190,10 +190,7 @@ ANALYZE [ VERBOSE ] [ <replaceable class="parameter">table_and_columns</replacea analyze all tables in their databases, except shared catalogs. (The restriction for shared catalogs means that a true database-wide <command>ANALYZE</command> can only be performed by superusers and roles - with privileges of <literal>pg_maintain</literal>.) If a role has - permission to <command>ANALYZE</command> a partitioned table, it is also - permitted to <command>ANALYZE</command> each of its partitions, regardless - of whether the role has the aforementioned privileges on the partition. + with privileges of <literal>pg_maintain</literal>.) <command>ANALYZE</command> will skip over any tables that the calling user does not have permission to analyze. </para> diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml index 29f0f1fd90..f0dd7faed5 100644 --- a/doc/src/sgml/ref/cluster.sgml +++ b/doc/src/sgml/ref/cluster.sgml @@ -137,10 +137,7 @@ CLUSTER [VERBOSE] on the table or be the table's owner, a superuser, or a role with privileges of the <link linkend="predefined-roles-table"><literal>pg_maintain</literal></link> - role. If a role has permission to <command>CLUSTER</command> a partitioned - table, it is also permitted to <command>CLUSTER</command> each of its - partitions, regardless of whether the role has the aforementioned - privileges on the partition. <command>CLUSTER</command> will skip over any + role. <command>CLUSTER</command> will skip over any tables that the calling user does not have permission to cluster. </para> diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml index 5b3b2b793a..8524182211 100644 --- a/doc/src/sgml/ref/lock.sgml +++ b/doc/src/sgml/ref/lock.sgml @@ -177,10 +177,7 @@ LOCK [ TABLE ] [ ONLY ] <replaceable class="parameter">name</replaceable> [ * ] MODE</literal> (or a less-conflicting mode as described in <xref linkend="explicit-locking"/>) is permitted. If a user has <literal>SELECT</literal> privileges on the table, <literal>ACCESS SHARE - MODE</literal> is permitted. If a role has permission to lock a - partitioned table, it is also permitted to lock each of its partitions, - regardless of whether the role has the aforementioned privileges on the - partition. + MODE</literal> is permitted. </para> <para> diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml index 71455dfdc7..23f8c7630b 100644 --- a/doc/src/sgml/ref/reindex.sgml +++ b/doc/src/sgml/ref/reindex.sgml @@ -306,11 +306,7 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { DA indexes on shared catalogs will be skipped unless the user owns the catalog (which typically won't be the case), has privileges of the <literal>pg_maintain</literal> role, or has the <literal>MAINTAIN</literal> - 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. </para> <para> diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml index 57bc4c23ec..445325e14c 100644 --- a/doc/src/sgml/ref/vacuum.sgml +++ b/doc/src/sgml/ref/vacuum.sgml @@ -452,10 +452,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet vacuum all tables in their databases, except shared catalogs. (The restriction for shared catalogs means that a true database-wide <command>VACUUM</command> can only be performed by superusers and roles - with privileges of <literal>pg_maintain</literal>.) If a role has - permission to <command>VACUUM</command> a partitioned table, it is also - permitted to <command>VACUUM</command> each of its partitions, regardless - of whether the role has the aforementioned privileges on the partition. + with privileges of <literal>pg_maintain</literal>.) <command>VACUUM</command> will skip over any tables that the calling user does not have permission to vacuum. </para> diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 369fea7c04..38834356b9 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -1693,11 +1693,8 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid) if (get_rel_relkind(indexrelid) != RELKIND_INDEX) continue; - /* - * 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; /* Use a permanent memory context for the result list */ old_context = MemoryContextSwitchTo(cluster_context); @@ -1720,8 +1717,7 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid) static bool cluster_is_permitted_for_relation(Oid relid, Oid userid) { - if (pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK || - has_partition_ancestor_privs(relid, userid, ACL_MAINTAIN)) + if (pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK) return true; ereport(WARNING, diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index a5168c9f09..9bc97e1fc2 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -2853,11 +2853,14 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation, /* Check permissions */ table_oid = IndexGetRelation(relId, true); - if (OidIsValid(table_oid) && - pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && - !has_partition_ancestor_privs(table_oid, GetUserId(), ACL_MAINTAIN)) - aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_INDEX, - relation->relname); + if (OidIsValid(table_oid)) + { + AclResult aclresult; + + aclresult = pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN); + if (aclresult != ACLCHECK_OK) + aclcheck_error(aclresult, OBJECT_INDEX, relation->relname); + } /* Lock heap before index to avoid deadlock. */ if (relId != oldRelId) @@ -3064,18 +3067,12 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, continue; /* - * The table can be reindexed if the user has been granted MAINTAIN on - * the table or one of its partition ancestors or the user is a - * superuser, the table owner, or the database/schema owner (but in - * the latter case, only if it's not a shared relation). - * pg_class_aclcheck includes the superuser case, and depending on - * objectKind we already know that the user has permission to run - * REINDEX on this database or schema per the permission checks at the - * beginning of this routine. + * We already checked privileges on the database or schema, but we + * further restrict reindexing shared catalogs to roles with the + * MAINTAIN privilege on the relation. */ if (classtuple->relisshared && - pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && - !has_partition_ancestor_privs(relid, GetUserId(), ACL_MAINTAIN)) + pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK) continue; /* diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c index 43c7d7f4bb..92662cbbc8 100644 --- a/src/backend/commands/lockcmds.c +++ b/src/backend/commands/lockcmds.c @@ -19,7 +19,6 @@ #include "catalog/namespace.h" #include "catalog/pg_inherits.h" #include "commands/lockcmds.h" -#include "commands/tablecmds.h" #include "miscadmin.h" #include "nodes/nodeFuncs.h" #include "parser/parse_clause.h" @@ -297,12 +296,5 @@ LockTableAclCheck(Oid reloid, LOCKMODE lockmode, Oid userid) aclresult = pg_class_aclcheck(reloid, userid, aclmask); - /* - * If this is a partition, check permissions of its ancestors if needed. - */ - if (aclresult != ACLCHECK_OK && - has_partition_ancestor_privs(reloid, userid, ACL_MAINTAIN)) - aclresult = ACLCHECK_OK; - return aclresult; } diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 4d49d70c33..9b12bc44d7 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -16986,6 +16986,7 @@ RangeVarCallbackMaintainsTable(const RangeVar *relation, Oid relId, Oid oldRelId, void *arg) { char relkind; + AclResult aclresult; /* Nothing to do if the relation was not found. */ if (!OidIsValid(relId)) @@ -17006,36 +17007,9 @@ RangeVarCallbackMaintainsTable(const RangeVar *relation, errmsg("\"%s\" is not a table or materialized view", relation->relname))); /* Check permissions */ - if (pg_class_aclcheck(relId, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && - !has_partition_ancestor_privs(relId, GetUserId(), ACL_MAINTAIN)) - aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_TABLE, - relation->relname); -} - -/* - * If relid is a partition, returns whether userid has any of the privileges - * specified in acl on any of its ancestors. Otherwise, returns false. - */ -bool -has_partition_ancestor_privs(Oid relid, Oid userid, AclMode acl) -{ - List *ancestors; - ListCell *lc; - - if (!get_rel_relispartition(relid)) - return false; - - ancestors = get_partition_ancestors(relid); - foreach(lc, ancestors) - { - Oid ancestor = lfirst_oid(lc); - - if (OidIsValid(ancestor) && - pg_class_aclcheck(ancestor, userid, acl) == ACLCHECK_OK) - return true; - } - - return false; + aclresult = pg_class_aclcheck(relId, GetUserId(), ACL_MAINTAIN); + if (aclresult != ACLCHECK_OK) + aclcheck_error(aclresult, OBJECT_TABLE, relation->relname); } /* diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index bb79de4da6..e7a0450cc4 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -41,7 +41,6 @@ #include "catalog/pg_namespace.h" #include "commands/cluster.h" #include "commands/defrem.h" -#include "commands/tablecmds.h" #include "commands/vacuum.h" #include "miscadmin.h" #include "nodes/makefuncs.h" @@ -721,17 +720,12 @@ vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple, /*---------- * A role has privileges to vacuum or analyze the relation if any of the * following are true: - * - the role is a superuser - * - the role owns the relation * - the role owns the current database and the relation is not shared * - the role has been granted the MAINTAIN privilege on the relation - * - the role has privileges to vacuum/analyze any of the relation's - * partition ancestors *---------- */ if ((object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) && !reltuple->relisshared) || - pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) == ACLCHECK_OK || - has_partition_ancestor_privs(relid, GetUserId(), ACL_MAINTAIN)) + pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) == ACLCHECK_OK) return true; relname = NameStr(reltuple->relname); diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h index 17b9404937..250d89ff88 100644 --- a/src/include/commands/tablecmds.h +++ b/src/include/commands/tablecmds.h @@ -99,7 +99,6 @@ extern void AtEOSubXact_on_commit_actions(bool isCommit, extern void RangeVarCallbackMaintainsTable(const RangeVar *relation, Oid relId, Oid oldRelId, void *arg); -extern bool has_partition_ancestor_privs(Oid relid, Oid userid, AclMode acl); extern void RangeVarCallbackOwnsRelation(const RangeVar *relation, Oid relId, Oid oldRelId, void *arg); diff --git a/src/test/isolation/expected/cluster-conflict-partition.out b/src/test/isolation/expected/cluster-conflict-partition.out index 8d21276996..7be9e56ef1 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; <waiting ...> step s1_commit: COMMIT; step s2_cluster: <... completed> @@ -11,7 +11,7 @@ step s2_reset: RESET ROLE; starting permutation: s1_begin s2_auth s1_lock_parent s2_cluster s1_commit s2_reset step s1_begin: BEGIN; -step s2_auth: SET ROLE regress_cluster_part; +step s2_auth: SET ROLE regress_cluster_part; SET client_min_messages = ERROR; step s1_lock_parent: LOCK cluster_part_tab IN SHARE UPDATE EXCLUSIVE MODE; step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...> step s1_commit: COMMIT; @@ -21,17 +21,15 @@ step s2_reset: RESET ROLE; starting permutation: s1_begin s1_lock_child s2_auth s2_cluster s1_commit s2_reset step s1_begin: BEGIN; step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; -step s2_auth: SET ROLE regress_cluster_part; -step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...> +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> step s2_reset: RESET ROLE; starting permutation: s1_begin s2_auth s1_lock_child s2_cluster s1_commit s2_reset step s1_begin: BEGIN; -step s2_auth: SET ROLE regress_cluster_part; +step s2_auth: SET ROLE regress_cluster_part; SET client_min_messages = ERROR; step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; -step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...> +step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; step s1_commit: COMMIT; -step s2_cluster: <... completed> step s2_reset: RESET ROLE; diff --git a/src/test/isolation/specs/cluster-conflict-partition.spec b/src/test/isolation/specs/cluster-conflict-partition.spec index ae38cb4ee3..4d38a7f49a 100644 --- a/src/test/isolation/specs/cluster-conflict-partition.spec +++ b/src/test/isolation/specs/cluster-conflict-partition.spec @@ -23,12 +23,15 @@ step s1_lock_child { LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; step s1_commit { COMMIT; } session s2 -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 s2_reset { RESET ROLE; } -# CLUSTER waits if locked, passes for all cases. +# CLUSTER on the parent waits if locked, passes for all cases. permutation s1_begin s1_lock_parent s2_auth s2_cluster s1_commit s2_reset permutation s1_begin s2_auth s1_lock_parent s2_cluster s1_commit s2_reset + +# When taking a lock on a partition leaf, CLUSTER on the parent skips +# the leaf, passes for all cases. permutation s1_begin s1_lock_child s2_auth s2_cluster s1_commit s2_reset permutation s1_begin s2_auth s1_lock_child s2_cluster s1_commit s2_reset diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out index 2eec483eaa..27a5dff5d4 100644 --- a/src/test/regress/expected/cluster.out +++ b/src/test/regress/expected/cluster.out @@ -508,6 +508,7 @@ CREATE TEMP TABLE ptnowner_oldnodes AS JOIN pg_class AS c ON c.oid=tree.relid; SET SESSION AUTHORIZATION regress_ptnowner; CLUSTER ptnowner USING ptnowner_i_idx; +WARNING: permission denied to cluster "ptnowner2", skipping it RESET SESSION AUTHORIZATION; SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a JOIN ptnowner_oldnodes b USING (oid) ORDER BY a.relname COLLATE "C"; @@ -515,7 +516,7 @@ SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a -----------+---------- ptnowner | t ptnowner1 | f - ptnowner2 | f + ptnowner2 | t (3 rows) DROP TABLE ptnowner; diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out index acfd9d1f4f..1473bc3175 100644 --- a/src/test/regress/expected/create_index.out +++ b/src/test/regress/expected/create_index.out @@ -2831,9 +2831,9 @@ RESET ROLE; GRANT USAGE ON SCHEMA pg_toast TO regress_reindexuser; SET SESSION ROLE regress_reindexuser; REINDEX TABLE pg_toast.pg_toast_1260; -ERROR: must be owner of table pg_toast_1260 +ERROR: permission denied for table pg_toast_1260 REINDEX INDEX pg_toast.pg_toast_1260_index; -ERROR: must be owner of index pg_toast_1260_index +ERROR: permission denied for index pg_toast_1260_index -- Clean up RESET ROLE; REVOKE USAGE ON SCHEMA pg_toast FROM regress_reindexuser; diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out index 3cf4ac8c9e..3e4dfcc2ec 100644 --- a/src/test/regress/expected/privileges.out +++ b/src/test/regress/expected/privileges.out @@ -2928,13 +2928,13 @@ WARNING: permission denied to analyze "maintain_test", skipping it VACUUM (ANALYZE) maintain_test; WARNING: permission denied to vacuum "maintain_test", skipping it CLUSTER maintain_test USING maintain_test_a_idx; -ERROR: must be owner of table maintain_test +ERROR: permission denied for table maintain_test REFRESH MATERIALIZED VIEW refresh_test; -ERROR: must be owner of table refresh_test +ERROR: permission denied for table refresh_test REINDEX TABLE maintain_test; -ERROR: must be owner of table maintain_test +ERROR: permission denied for table maintain_test REINDEX INDEX maintain_test_a_idx; -ERROR: must be owner of index maintain_test_a_idx +ERROR: permission denied for index maintain_test_a_idx REINDEX SCHEMA reindex_test; ERROR: must be owner of schema reindex_test RESET ROLE; diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out index 41e020cf20..4def90b805 100644 --- a/src/test/regress/expected/vacuum.out +++ b/src/test/regress/expected/vacuum.out @@ -442,14 +442,20 @@ ALTER TABLE vacowned_parted OWNER TO regress_vacuum; ALTER TABLE vacowned_part1 OWNER TO regress_vacuum; SET ROLE regress_vacuum; VACUUM vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM vacowned_part1; VACUUM vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it ANALYZE vacowned_parted; +WARNING: permission denied to analyze "vacowned_part2", skipping it ANALYZE vacowned_part1; ANALYZE vacowned_part2; +WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; VACUUM (ANALYZE) vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it RESET ROLE; -- Only one partition owned by other user. ALTER TABLE vacowned_parted OWNER TO CURRENT_USER; @@ -478,14 +484,26 @@ ALTER TABLE vacowned_parted OWNER TO regress_vacuum; ALTER TABLE vacowned_part1 OWNER TO CURRENT_USER; SET ROLE regress_vacuum; VACUUM vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part1", skipping it +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM vacowned_part1; +WARNING: permission denied to vacuum "vacowned_part1", skipping it VACUUM vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it ANALYZE vacowned_parted; +WARNING: permission denied to analyze "vacowned_part1", skipping it +WARNING: permission denied to analyze "vacowned_part2", skipping it ANALYZE vacowned_part1; +WARNING: permission denied to analyze "vacowned_part1", skipping it ANALYZE vacowned_part2; +WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part1", skipping it +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; +WARNING: permission denied to vacuum "vacowned_part1", skipping it VACUUM (ANALYZE) vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it RESET ROLE; DROP TABLE vacowned; DROP TABLE vacowned_parted; -- 2.25.1 --/9DWx/yDrRhgMJTb Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v7-0002-simplify-privilege-related-documentation-for-main.patch" ^ permalink raw reply [nested|flat] 8+ messages in thread
* [PATCH v8 1/2] partial revert of ff9618e82a @ 2023-06-19 20:57 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Nathan Bossart @ 2023-06-19 20:57 UTC (permalink / raw) --- doc/src/sgml/ref/analyze.sgml | 5 +-- doc/src/sgml/ref/cluster.sgml | 5 +-- doc/src/sgml/ref/lock.sgml | 5 +-- doc/src/sgml/ref/reindex.sgml | 6 +--- doc/src/sgml/ref/vacuum.sgml | 5 +-- src/backend/commands/cluster.c | 10 ++---- src/backend/commands/indexcmds.c | 27 +++++++-------- src/backend/commands/lockcmds.c | 8 ----- src/backend/commands/tablecmds.c | 34 +++---------------- src/backend/commands/vacuum.c | 8 +---- src/include/commands/tablecmds.h | 1 - .../expected/cluster-conflict-partition.out | 14 ++++---- .../specs/cluster-conflict-partition.spec | 7 ++-- src/test/regress/expected/cluster.out | 3 +- src/test/regress/expected/create_index.out | 4 +-- src/test/regress/expected/privileges.out | 8 ++--- src/test/regress/expected/vacuum.out | 18 ++++++++++ 17 files changed, 62 insertions(+), 106 deletions(-) diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml index 20c6f9939f..30a893230e 100644 --- a/doc/src/sgml/ref/analyze.sgml +++ b/doc/src/sgml/ref/analyze.sgml @@ -190,10 +190,7 @@ ANALYZE [ VERBOSE ] [ <replaceable class="parameter">table_and_columns</replacea analyze all tables in their databases, except shared catalogs. (The restriction for shared catalogs means that a true database-wide <command>ANALYZE</command> can only be performed by superusers and roles - with privileges of <literal>pg_maintain</literal>.) If a role has - permission to <command>ANALYZE</command> a partitioned table, it is also - permitted to <command>ANALYZE</command> each of its partitions, regardless - of whether the role has the aforementioned privileges on the partition. + with privileges of <literal>pg_maintain</literal>.) <command>ANALYZE</command> will skip over any tables that the calling user does not have permission to analyze. </para> diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml index 29f0f1fd90..f0dd7faed5 100644 --- a/doc/src/sgml/ref/cluster.sgml +++ b/doc/src/sgml/ref/cluster.sgml @@ -137,10 +137,7 @@ CLUSTER [VERBOSE] on the table or be the table's owner, a superuser, or a role with privileges of the <link linkend="predefined-roles-table"><literal>pg_maintain</literal></link> - role. If a role has permission to <command>CLUSTER</command> a partitioned - table, it is also permitted to <command>CLUSTER</command> each of its - partitions, regardless of whether the role has the aforementioned - privileges on the partition. <command>CLUSTER</command> will skip over any + role. <command>CLUSTER</command> will skip over any tables that the calling user does not have permission to cluster. </para> diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml index 5b3b2b793a..8524182211 100644 --- a/doc/src/sgml/ref/lock.sgml +++ b/doc/src/sgml/ref/lock.sgml @@ -177,10 +177,7 @@ LOCK [ TABLE ] [ ONLY ] <replaceable class="parameter">name</replaceable> [ * ] MODE</literal> (or a less-conflicting mode as described in <xref linkend="explicit-locking"/>) is permitted. If a user has <literal>SELECT</literal> privileges on the table, <literal>ACCESS SHARE - MODE</literal> is permitted. If a role has permission to lock a - partitioned table, it is also permitted to lock each of its partitions, - regardless of whether the role has the aforementioned privileges on the - partition. + MODE</literal> is permitted. </para> <para> diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml index 71455dfdc7..23f8c7630b 100644 --- a/doc/src/sgml/ref/reindex.sgml +++ b/doc/src/sgml/ref/reindex.sgml @@ -306,11 +306,7 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { DA indexes on shared catalogs will be skipped unless the user owns the catalog (which typically won't be the case), has privileges of the <literal>pg_maintain</literal> role, or has the <literal>MAINTAIN</literal> - 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. </para> <para> diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml index 57bc4c23ec..445325e14c 100644 --- a/doc/src/sgml/ref/vacuum.sgml +++ b/doc/src/sgml/ref/vacuum.sgml @@ -452,10 +452,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet vacuum all tables in their databases, except shared catalogs. (The restriction for shared catalogs means that a true database-wide <command>VACUUM</command> can only be performed by superusers and roles - with privileges of <literal>pg_maintain</literal>.) If a role has - permission to <command>VACUUM</command> a partitioned table, it is also - permitted to <command>VACUUM</command> each of its partitions, regardless - of whether the role has the aforementioned privileges on the partition. + with privileges of <literal>pg_maintain</literal>.) <command>VACUUM</command> will skip over any tables that the calling user does not have permission to vacuum. </para> diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 369fea7c04..38834356b9 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -1693,11 +1693,8 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid) if (get_rel_relkind(indexrelid) != RELKIND_INDEX) continue; - /* - * 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; /* Use a permanent memory context for the result list */ old_context = MemoryContextSwitchTo(cluster_context); @@ -1720,8 +1717,7 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid) static bool cluster_is_permitted_for_relation(Oid relid, Oid userid) { - if (pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK || - has_partition_ancestor_privs(relid, userid, ACL_MAINTAIN)) + if (pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK) return true; ereport(WARNING, diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index a5168c9f09..9bc97e1fc2 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -2853,11 +2853,14 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation, /* Check permissions */ table_oid = IndexGetRelation(relId, true); - if (OidIsValid(table_oid) && - pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && - !has_partition_ancestor_privs(table_oid, GetUserId(), ACL_MAINTAIN)) - aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_INDEX, - relation->relname); + if (OidIsValid(table_oid)) + { + AclResult aclresult; + + aclresult = pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN); + if (aclresult != ACLCHECK_OK) + aclcheck_error(aclresult, OBJECT_INDEX, relation->relname); + } /* Lock heap before index to avoid deadlock. */ if (relId != oldRelId) @@ -3064,18 +3067,12 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, continue; /* - * The table can be reindexed if the user has been granted MAINTAIN on - * the table or one of its partition ancestors or the user is a - * superuser, the table owner, or the database/schema owner (but in - * the latter case, only if it's not a shared relation). - * pg_class_aclcheck includes the superuser case, and depending on - * objectKind we already know that the user has permission to run - * REINDEX on this database or schema per the permission checks at the - * beginning of this routine. + * We already checked privileges on the database or schema, but we + * further restrict reindexing shared catalogs to roles with the + * MAINTAIN privilege on the relation. */ if (classtuple->relisshared && - pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && - !has_partition_ancestor_privs(relid, GetUserId(), ACL_MAINTAIN)) + pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK) continue; /* diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c index 43c7d7f4bb..92662cbbc8 100644 --- a/src/backend/commands/lockcmds.c +++ b/src/backend/commands/lockcmds.c @@ -19,7 +19,6 @@ #include "catalog/namespace.h" #include "catalog/pg_inherits.h" #include "commands/lockcmds.h" -#include "commands/tablecmds.h" #include "miscadmin.h" #include "nodes/nodeFuncs.h" #include "parser/parse_clause.h" @@ -297,12 +296,5 @@ LockTableAclCheck(Oid reloid, LOCKMODE lockmode, Oid userid) aclresult = pg_class_aclcheck(reloid, userid, aclmask); - /* - * If this is a partition, check permissions of its ancestors if needed. - */ - if (aclresult != ACLCHECK_OK && - has_partition_ancestor_privs(reloid, userid, ACL_MAINTAIN)) - aclresult = ACLCHECK_OK; - return aclresult; } diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 4d49d70c33..9b12bc44d7 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -16986,6 +16986,7 @@ RangeVarCallbackMaintainsTable(const RangeVar *relation, Oid relId, Oid oldRelId, void *arg) { char relkind; + AclResult aclresult; /* Nothing to do if the relation was not found. */ if (!OidIsValid(relId)) @@ -17006,36 +17007,9 @@ RangeVarCallbackMaintainsTable(const RangeVar *relation, errmsg("\"%s\" is not a table or materialized view", relation->relname))); /* Check permissions */ - if (pg_class_aclcheck(relId, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && - !has_partition_ancestor_privs(relId, GetUserId(), ACL_MAINTAIN)) - aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_TABLE, - relation->relname); -} - -/* - * If relid is a partition, returns whether userid has any of the privileges - * specified in acl on any of its ancestors. Otherwise, returns false. - */ -bool -has_partition_ancestor_privs(Oid relid, Oid userid, AclMode acl) -{ - List *ancestors; - ListCell *lc; - - if (!get_rel_relispartition(relid)) - return false; - - ancestors = get_partition_ancestors(relid); - foreach(lc, ancestors) - { - Oid ancestor = lfirst_oid(lc); - - if (OidIsValid(ancestor) && - pg_class_aclcheck(ancestor, userid, acl) == ACLCHECK_OK) - return true; - } - - return false; + aclresult = pg_class_aclcheck(relId, GetUserId(), ACL_MAINTAIN); + if (aclresult != ACLCHECK_OK) + aclcheck_error(aclresult, OBJECT_TABLE, relation->relname); } /* diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index bb79de4da6..e7a0450cc4 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -41,7 +41,6 @@ #include "catalog/pg_namespace.h" #include "commands/cluster.h" #include "commands/defrem.h" -#include "commands/tablecmds.h" #include "commands/vacuum.h" #include "miscadmin.h" #include "nodes/makefuncs.h" @@ -721,17 +720,12 @@ vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple, /*---------- * A role has privileges to vacuum or analyze the relation if any of the * following are true: - * - the role is a superuser - * - the role owns the relation * - the role owns the current database and the relation is not shared * - the role has been granted the MAINTAIN privilege on the relation - * - the role has privileges to vacuum/analyze any of the relation's - * partition ancestors *---------- */ if ((object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) && !reltuple->relisshared) || - pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) == ACLCHECK_OK || - has_partition_ancestor_privs(relid, GetUserId(), ACL_MAINTAIN)) + pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) == ACLCHECK_OK) return true; relname = NameStr(reltuple->relname); diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h index 17b9404937..250d89ff88 100644 --- a/src/include/commands/tablecmds.h +++ b/src/include/commands/tablecmds.h @@ -99,7 +99,6 @@ extern void AtEOSubXact_on_commit_actions(bool isCommit, extern void RangeVarCallbackMaintainsTable(const RangeVar *relation, Oid relId, Oid oldRelId, void *arg); -extern bool has_partition_ancestor_privs(Oid relid, Oid userid, AclMode acl); extern void RangeVarCallbackOwnsRelation(const RangeVar *relation, Oid relId, Oid oldRelId, void *arg); diff --git a/src/test/isolation/expected/cluster-conflict-partition.out b/src/test/isolation/expected/cluster-conflict-partition.out index 8d21276996..7be9e56ef1 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; <waiting ...> step s1_commit: COMMIT; step s2_cluster: <... completed> @@ -11,7 +11,7 @@ step s2_reset: RESET ROLE; starting permutation: s1_begin s2_auth s1_lock_parent s2_cluster s1_commit s2_reset step s1_begin: BEGIN; -step s2_auth: SET ROLE regress_cluster_part; +step s2_auth: SET ROLE regress_cluster_part; SET client_min_messages = ERROR; step s1_lock_parent: LOCK cluster_part_tab IN SHARE UPDATE EXCLUSIVE MODE; step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...> step s1_commit: COMMIT; @@ -21,17 +21,15 @@ step s2_reset: RESET ROLE; starting permutation: s1_begin s1_lock_child s2_auth s2_cluster s1_commit s2_reset step s1_begin: BEGIN; step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; -step s2_auth: SET ROLE regress_cluster_part; -step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...> +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> step s2_reset: RESET ROLE; starting permutation: s1_begin s2_auth s1_lock_child s2_cluster s1_commit s2_reset step s1_begin: BEGIN; -step s2_auth: SET ROLE regress_cluster_part; +step s2_auth: SET ROLE regress_cluster_part; SET client_min_messages = ERROR; step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; -step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...> +step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; step s1_commit: COMMIT; -step s2_cluster: <... completed> step s2_reset: RESET ROLE; diff --git a/src/test/isolation/specs/cluster-conflict-partition.spec b/src/test/isolation/specs/cluster-conflict-partition.spec index ae38cb4ee3..4d38a7f49a 100644 --- a/src/test/isolation/specs/cluster-conflict-partition.spec +++ b/src/test/isolation/specs/cluster-conflict-partition.spec @@ -23,12 +23,15 @@ step s1_lock_child { LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; step s1_commit { COMMIT; } session s2 -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 s2_reset { RESET ROLE; } -# CLUSTER waits if locked, passes for all cases. +# CLUSTER on the parent waits if locked, passes for all cases. permutation s1_begin s1_lock_parent s2_auth s2_cluster s1_commit s2_reset permutation s1_begin s2_auth s1_lock_parent s2_cluster s1_commit s2_reset + +# When taking a lock on a partition leaf, CLUSTER on the parent skips +# the leaf, passes for all cases. permutation s1_begin s1_lock_child s2_auth s2_cluster s1_commit s2_reset permutation s1_begin s2_auth s1_lock_child s2_cluster s1_commit s2_reset diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out index 2eec483eaa..27a5dff5d4 100644 --- a/src/test/regress/expected/cluster.out +++ b/src/test/regress/expected/cluster.out @@ -508,6 +508,7 @@ CREATE TEMP TABLE ptnowner_oldnodes AS JOIN pg_class AS c ON c.oid=tree.relid; SET SESSION AUTHORIZATION regress_ptnowner; CLUSTER ptnowner USING ptnowner_i_idx; +WARNING: permission denied to cluster "ptnowner2", skipping it RESET SESSION AUTHORIZATION; SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a JOIN ptnowner_oldnodes b USING (oid) ORDER BY a.relname COLLATE "C"; @@ -515,7 +516,7 @@ SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a -----------+---------- ptnowner | t ptnowner1 | f - ptnowner2 | f + ptnowner2 | t (3 rows) DROP TABLE ptnowner; diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out index acfd9d1f4f..1473bc3175 100644 --- a/src/test/regress/expected/create_index.out +++ b/src/test/regress/expected/create_index.out @@ -2831,9 +2831,9 @@ RESET ROLE; GRANT USAGE ON SCHEMA pg_toast TO regress_reindexuser; SET SESSION ROLE regress_reindexuser; REINDEX TABLE pg_toast.pg_toast_1260; -ERROR: must be owner of table pg_toast_1260 +ERROR: permission denied for table pg_toast_1260 REINDEX INDEX pg_toast.pg_toast_1260_index; -ERROR: must be owner of index pg_toast_1260_index +ERROR: permission denied for index pg_toast_1260_index -- Clean up RESET ROLE; REVOKE USAGE ON SCHEMA pg_toast FROM regress_reindexuser; diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out index 3cf4ac8c9e..3e4dfcc2ec 100644 --- a/src/test/regress/expected/privileges.out +++ b/src/test/regress/expected/privileges.out @@ -2928,13 +2928,13 @@ WARNING: permission denied to analyze "maintain_test", skipping it VACUUM (ANALYZE) maintain_test; WARNING: permission denied to vacuum "maintain_test", skipping it CLUSTER maintain_test USING maintain_test_a_idx; -ERROR: must be owner of table maintain_test +ERROR: permission denied for table maintain_test REFRESH MATERIALIZED VIEW refresh_test; -ERROR: must be owner of table refresh_test +ERROR: permission denied for table refresh_test REINDEX TABLE maintain_test; -ERROR: must be owner of table maintain_test +ERROR: permission denied for table maintain_test REINDEX INDEX maintain_test_a_idx; -ERROR: must be owner of index maintain_test_a_idx +ERROR: permission denied for index maintain_test_a_idx REINDEX SCHEMA reindex_test; ERROR: must be owner of schema reindex_test RESET ROLE; diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out index 41e020cf20..4def90b805 100644 --- a/src/test/regress/expected/vacuum.out +++ b/src/test/regress/expected/vacuum.out @@ -442,14 +442,20 @@ ALTER TABLE vacowned_parted OWNER TO regress_vacuum; ALTER TABLE vacowned_part1 OWNER TO regress_vacuum; SET ROLE regress_vacuum; VACUUM vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM vacowned_part1; VACUUM vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it ANALYZE vacowned_parted; +WARNING: permission denied to analyze "vacowned_part2", skipping it ANALYZE vacowned_part1; ANALYZE vacowned_part2; +WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; VACUUM (ANALYZE) vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it RESET ROLE; -- Only one partition owned by other user. ALTER TABLE vacowned_parted OWNER TO CURRENT_USER; @@ -478,14 +484,26 @@ ALTER TABLE vacowned_parted OWNER TO regress_vacuum; ALTER TABLE vacowned_part1 OWNER TO CURRENT_USER; SET ROLE regress_vacuum; VACUUM vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part1", skipping it +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM vacowned_part1; +WARNING: permission denied to vacuum "vacowned_part1", skipping it VACUUM vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it ANALYZE vacowned_parted; +WARNING: permission denied to analyze "vacowned_part1", skipping it +WARNING: permission denied to analyze "vacowned_part2", skipping it ANALYZE vacowned_part1; +WARNING: permission denied to analyze "vacowned_part1", skipping it ANALYZE vacowned_part2; +WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part1", skipping it +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; +WARNING: permission denied to vacuum "vacowned_part1", skipping it VACUUM (ANALYZE) vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it RESET ROLE; DROP TABLE vacowned; DROP TABLE vacowned_parted; -- 2.25.1 --FL5UXtIhxfXey3p5 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v8-0002-simplify-privilege-related-documentation-for-main.patch" ^ permalink raw reply [nested|flat] 8+ messages in thread
* [PATCH v9 1/2] partial revert of ff9618e82a @ 2023-06-19 20:57 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Nathan Bossart @ 2023-06-19 20:57 UTC (permalink / raw) --- doc/src/sgml/ref/analyze.sgml | 5 +-- doc/src/sgml/ref/cluster.sgml | 5 +-- doc/src/sgml/ref/lock.sgml | 5 +-- doc/src/sgml/ref/reindex.sgml | 6 +--- doc/src/sgml/ref/vacuum.sgml | 5 +-- src/backend/commands/cluster.c | 12 ++++--- src/backend/commands/indexcmds.c | 27 +++++++-------- src/backend/commands/lockcmds.c | 8 ----- src/backend/commands/tablecmds.c | 34 +++---------------- src/backend/commands/vacuum.c | 10 ++---- src/include/commands/tablecmds.h | 1 - .../expected/cluster-conflict-partition.out | 14 ++++---- .../specs/cluster-conflict-partition.spec | 7 ++-- src/test/regress/expected/cluster.out | 7 +++- src/test/regress/expected/create_index.out | 4 +-- src/test/regress/expected/privileges.out | 8 ++--- src/test/regress/expected/vacuum.out | 18 ++++++++++ src/test/regress/sql/cluster.sql | 3 ++ 18 files changed, 74 insertions(+), 105 deletions(-) diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml index 20c6f9939f..30a893230e 100644 --- a/doc/src/sgml/ref/analyze.sgml +++ b/doc/src/sgml/ref/analyze.sgml @@ -190,10 +190,7 @@ ANALYZE [ VERBOSE ] [ <replaceable class="parameter">table_and_columns</replacea analyze all tables in their databases, except shared catalogs. (The restriction for shared catalogs means that a true database-wide <command>ANALYZE</command> can only be performed by superusers and roles - with privileges of <literal>pg_maintain</literal>.) If a role has - permission to <command>ANALYZE</command> a partitioned table, it is also - permitted to <command>ANALYZE</command> each of its partitions, regardless - of whether the role has the aforementioned privileges on the partition. + with privileges of <literal>pg_maintain</literal>.) <command>ANALYZE</command> will skip over any tables that the calling user does not have permission to analyze. </para> diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml index 29f0f1fd90..f0dd7faed5 100644 --- a/doc/src/sgml/ref/cluster.sgml +++ b/doc/src/sgml/ref/cluster.sgml @@ -137,10 +137,7 @@ CLUSTER [VERBOSE] on the table or be the table's owner, a superuser, or a role with privileges of the <link linkend="predefined-roles-table"><literal>pg_maintain</literal></link> - role. If a role has permission to <command>CLUSTER</command> a partitioned - table, it is also permitted to <command>CLUSTER</command> each of its - partitions, regardless of whether the role has the aforementioned - privileges on the partition. <command>CLUSTER</command> will skip over any + role. <command>CLUSTER</command> will skip over any tables that the calling user does not have permission to cluster. </para> diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml index 5b3b2b793a..8524182211 100644 --- a/doc/src/sgml/ref/lock.sgml +++ b/doc/src/sgml/ref/lock.sgml @@ -177,10 +177,7 @@ LOCK [ TABLE ] [ ONLY ] <replaceable class="parameter">name</replaceable> [ * ] MODE</literal> (or a less-conflicting mode as described in <xref linkend="explicit-locking"/>) is permitted. If a user has <literal>SELECT</literal> privileges on the table, <literal>ACCESS SHARE - MODE</literal> is permitted. If a role has permission to lock a - partitioned table, it is also permitted to lock each of its partitions, - regardless of whether the role has the aforementioned privileges on the - partition. + MODE</literal> is permitted. </para> <para> diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml index 71455dfdc7..23f8c7630b 100644 --- a/doc/src/sgml/ref/reindex.sgml +++ b/doc/src/sgml/ref/reindex.sgml @@ -306,11 +306,7 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { DA indexes on shared catalogs will be skipped unless the user owns the catalog (which typically won't be the case), has privileges of the <literal>pg_maintain</literal> role, or has the <literal>MAINTAIN</literal> - 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. </para> <para> diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml index 57bc4c23ec..445325e14c 100644 --- a/doc/src/sgml/ref/vacuum.sgml +++ b/doc/src/sgml/ref/vacuum.sgml @@ -452,10 +452,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet vacuum all tables in their databases, except shared catalogs. (The restriction for shared catalogs means that a true database-wide <command>VACUUM</command> can only be performed by superusers and roles - with privileges of <literal>pg_maintain</literal>.) If a role has - permission to <command>VACUUM</command> a partitioned table, it is also - permitted to <command>VACUUM</command> each of its partitions, regardless - of whether the role has the aforementioned privileges on the partition. + with privileges of <literal>pg_maintain</literal>.) <command>VACUUM</command> will skip over any tables that the calling user does not have permission to vacuum. </para> diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 369fea7c04..3bfabb6d10 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -1694,10 +1694,13 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid) continue; /* - * 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. + * 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. */ + if (!cluster_is_permitted_for_relation(relid, GetUserId())) + continue; /* Use a permanent memory context for the result list */ old_context = MemoryContextSwitchTo(cluster_context); @@ -1720,8 +1723,7 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid) static bool cluster_is_permitted_for_relation(Oid relid, Oid userid) { - if (pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK || - has_partition_ancestor_privs(relid, userid, ACL_MAINTAIN)) + if (pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK) return true; ereport(WARNING, diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index a5168c9f09..9bc97e1fc2 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -2853,11 +2853,14 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation, /* Check permissions */ table_oid = IndexGetRelation(relId, true); - if (OidIsValid(table_oid) && - pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && - !has_partition_ancestor_privs(table_oid, GetUserId(), ACL_MAINTAIN)) - aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_INDEX, - relation->relname); + if (OidIsValid(table_oid)) + { + AclResult aclresult; + + aclresult = pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN); + if (aclresult != ACLCHECK_OK) + aclcheck_error(aclresult, OBJECT_INDEX, relation->relname); + } /* Lock heap before index to avoid deadlock. */ if (relId != oldRelId) @@ -3064,18 +3067,12 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, continue; /* - * The table can be reindexed if the user has been granted MAINTAIN on - * the table or one of its partition ancestors or the user is a - * superuser, the table owner, or the database/schema owner (but in - * the latter case, only if it's not a shared relation). - * pg_class_aclcheck includes the superuser case, and depending on - * objectKind we already know that the user has permission to run - * REINDEX on this database or schema per the permission checks at the - * beginning of this routine. + * We already checked privileges on the database or schema, but we + * further restrict reindexing shared catalogs to roles with the + * MAINTAIN privilege on the relation. */ if (classtuple->relisshared && - pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && - !has_partition_ancestor_privs(relid, GetUserId(), ACL_MAINTAIN)) + pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK) continue; /* diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c index 43c7d7f4bb..92662cbbc8 100644 --- a/src/backend/commands/lockcmds.c +++ b/src/backend/commands/lockcmds.c @@ -19,7 +19,6 @@ #include "catalog/namespace.h" #include "catalog/pg_inherits.h" #include "commands/lockcmds.h" -#include "commands/tablecmds.h" #include "miscadmin.h" #include "nodes/nodeFuncs.h" #include "parser/parse_clause.h" @@ -297,12 +296,5 @@ LockTableAclCheck(Oid reloid, LOCKMODE lockmode, Oid userid) aclresult = pg_class_aclcheck(reloid, userid, aclmask); - /* - * If this is a partition, check permissions of its ancestors if needed. - */ - if (aclresult != ACLCHECK_OK && - has_partition_ancestor_privs(reloid, userid, ACL_MAINTAIN)) - aclresult = ACLCHECK_OK; - return aclresult; } diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 4d49d70c33..9b12bc44d7 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -16986,6 +16986,7 @@ RangeVarCallbackMaintainsTable(const RangeVar *relation, Oid relId, Oid oldRelId, void *arg) { char relkind; + AclResult aclresult; /* Nothing to do if the relation was not found. */ if (!OidIsValid(relId)) @@ -17006,36 +17007,9 @@ RangeVarCallbackMaintainsTable(const RangeVar *relation, errmsg("\"%s\" is not a table or materialized view", relation->relname))); /* Check permissions */ - if (pg_class_aclcheck(relId, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK && - !has_partition_ancestor_privs(relId, GetUserId(), ACL_MAINTAIN)) - aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_TABLE, - relation->relname); -} - -/* - * If relid is a partition, returns whether userid has any of the privileges - * specified in acl on any of its ancestors. Otherwise, returns false. - */ -bool -has_partition_ancestor_privs(Oid relid, Oid userid, AclMode acl) -{ - List *ancestors; - ListCell *lc; - - if (!get_rel_relispartition(relid)) - return false; - - ancestors = get_partition_ancestors(relid); - foreach(lc, ancestors) - { - Oid ancestor = lfirst_oid(lc); - - if (OidIsValid(ancestor) && - pg_class_aclcheck(ancestor, userid, acl) == ACLCHECK_OK) - return true; - } - - return false; + aclresult = pg_class_aclcheck(relId, GetUserId(), ACL_MAINTAIN); + if (aclresult != ACLCHECK_OK) + aclcheck_error(aclresult, OBJECT_TABLE, relation->relname); } /* diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index bb79de4da6..7fe6a54c06 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -41,7 +41,6 @@ #include "catalog/pg_namespace.h" #include "commands/cluster.h" #include "commands/defrem.h" -#include "commands/tablecmds.h" #include "commands/vacuum.h" #include "miscadmin.h" #include "nodes/makefuncs.h" @@ -721,17 +720,12 @@ vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple, /*---------- * A role has privileges to vacuum or analyze the relation if any of the * following are true: - * - the role is a superuser - * - the role owns the relation * - the role owns the current database and the relation is not shared - * - the role has been granted the MAINTAIN privilege on the relation - * - the role has privileges to vacuum/analyze any of the relation's - * partition ancestors + * - the role has the MAINTAIN privilege on the relation *---------- */ if ((object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) && !reltuple->relisshared) || - pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) == ACLCHECK_OK || - has_partition_ancestor_privs(relid, GetUserId(), ACL_MAINTAIN)) + pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) == ACLCHECK_OK) return true; relname = NameStr(reltuple->relname); diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h index 17b9404937..250d89ff88 100644 --- a/src/include/commands/tablecmds.h +++ b/src/include/commands/tablecmds.h @@ -99,7 +99,6 @@ extern void AtEOSubXact_on_commit_actions(bool isCommit, extern void RangeVarCallbackMaintainsTable(const RangeVar *relation, Oid relId, Oid oldRelId, void *arg); -extern bool has_partition_ancestor_privs(Oid relid, Oid userid, AclMode acl); extern void RangeVarCallbackOwnsRelation(const RangeVar *relation, Oid relId, Oid oldRelId, void *arg); diff --git a/src/test/isolation/expected/cluster-conflict-partition.out b/src/test/isolation/expected/cluster-conflict-partition.out index 8d21276996..7be9e56ef1 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; <waiting ...> step s1_commit: COMMIT; step s2_cluster: <... completed> @@ -11,7 +11,7 @@ step s2_reset: RESET ROLE; starting permutation: s1_begin s2_auth s1_lock_parent s2_cluster s1_commit s2_reset step s1_begin: BEGIN; -step s2_auth: SET ROLE regress_cluster_part; +step s2_auth: SET ROLE regress_cluster_part; SET client_min_messages = ERROR; step s1_lock_parent: LOCK cluster_part_tab IN SHARE UPDATE EXCLUSIVE MODE; step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...> step s1_commit: COMMIT; @@ -21,17 +21,15 @@ step s2_reset: RESET ROLE; starting permutation: s1_begin s1_lock_child s2_auth s2_cluster s1_commit s2_reset step s1_begin: BEGIN; step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; -step s2_auth: SET ROLE regress_cluster_part; -step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...> +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> step s2_reset: RESET ROLE; starting permutation: s1_begin s2_auth s1_lock_child s2_cluster s1_commit s2_reset step s1_begin: BEGIN; -step s2_auth: SET ROLE regress_cluster_part; +step s2_auth: SET ROLE regress_cluster_part; SET client_min_messages = ERROR; step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; -step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...> +step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; step s1_commit: COMMIT; -step s2_cluster: <... completed> step s2_reset: RESET ROLE; diff --git a/src/test/isolation/specs/cluster-conflict-partition.spec b/src/test/isolation/specs/cluster-conflict-partition.spec index ae38cb4ee3..4d38a7f49a 100644 --- a/src/test/isolation/specs/cluster-conflict-partition.spec +++ b/src/test/isolation/specs/cluster-conflict-partition.spec @@ -23,12 +23,15 @@ step s1_lock_child { LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; step s1_commit { COMMIT; } session s2 -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 s2_reset { RESET ROLE; } -# CLUSTER waits if locked, passes for all cases. +# CLUSTER on the parent waits if locked, passes for all cases. permutation s1_begin s1_lock_parent s2_auth s2_cluster s1_commit s2_reset permutation s1_begin s2_auth s1_lock_parent s2_cluster s1_commit s2_reset + +# When taking a lock on a partition leaf, CLUSTER on the parent skips +# the leaf, passes for all cases. permutation s1_begin s1_lock_child s2_auth s2_cluster s1_commit s2_reset permutation s1_begin s2_auth s1_lock_child s2_cluster s1_commit s2_reset diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out index 2eec483eaa..a13aafff0b 100644 --- a/src/test/regress/expected/cluster.out +++ b/src/test/regress/expected/cluster.out @@ -502,12 +502,17 @@ CREATE TABLE ptnowner1 PARTITION OF ptnowner FOR VALUES IN (1); CREATE ROLE regress_ptnowner; CREATE TABLE ptnowner2 PARTITION OF ptnowner FOR VALUES IN (2); ALTER TABLE ptnowner1 OWNER TO regress_ptnowner; +SET SESSION AUTHORIZATION regress_ptnowner; +CLUSTER ptnowner USING ptnowner_i_idx; +ERROR: permission denied for table ptnowner +RESET SESSION AUTHORIZATION; ALTER TABLE ptnowner OWNER TO regress_ptnowner; CREATE TEMP TABLE ptnowner_oldnodes AS SELECT oid, relname, relfilenode FROM pg_partition_tree('ptnowner') AS tree JOIN pg_class AS c ON c.oid=tree.relid; SET SESSION AUTHORIZATION regress_ptnowner; CLUSTER ptnowner USING ptnowner_i_idx; +WARNING: permission denied to cluster "ptnowner2", skipping it RESET SESSION AUTHORIZATION; SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a JOIN ptnowner_oldnodes b USING (oid) ORDER BY a.relname COLLATE "C"; @@ -515,7 +520,7 @@ SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a -----------+---------- ptnowner | t ptnowner1 | f - ptnowner2 | f + ptnowner2 | t (3 rows) DROP TABLE ptnowner; diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out index acfd9d1f4f..1473bc3175 100644 --- a/src/test/regress/expected/create_index.out +++ b/src/test/regress/expected/create_index.out @@ -2831,9 +2831,9 @@ RESET ROLE; GRANT USAGE ON SCHEMA pg_toast TO regress_reindexuser; SET SESSION ROLE regress_reindexuser; REINDEX TABLE pg_toast.pg_toast_1260; -ERROR: must be owner of table pg_toast_1260 +ERROR: permission denied for table pg_toast_1260 REINDEX INDEX pg_toast.pg_toast_1260_index; -ERROR: must be owner of index pg_toast_1260_index +ERROR: permission denied for index pg_toast_1260_index -- Clean up RESET ROLE; REVOKE USAGE ON SCHEMA pg_toast FROM regress_reindexuser; diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out index 3cf4ac8c9e..3e4dfcc2ec 100644 --- a/src/test/regress/expected/privileges.out +++ b/src/test/regress/expected/privileges.out @@ -2928,13 +2928,13 @@ WARNING: permission denied to analyze "maintain_test", skipping it VACUUM (ANALYZE) maintain_test; WARNING: permission denied to vacuum "maintain_test", skipping it CLUSTER maintain_test USING maintain_test_a_idx; -ERROR: must be owner of table maintain_test +ERROR: permission denied for table maintain_test REFRESH MATERIALIZED VIEW refresh_test; -ERROR: must be owner of table refresh_test +ERROR: permission denied for table refresh_test REINDEX TABLE maintain_test; -ERROR: must be owner of table maintain_test +ERROR: permission denied for table maintain_test REINDEX INDEX maintain_test_a_idx; -ERROR: must be owner of index maintain_test_a_idx +ERROR: permission denied for index maintain_test_a_idx REINDEX SCHEMA reindex_test; ERROR: must be owner of schema reindex_test RESET ROLE; diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out index 41e020cf20..4def90b805 100644 --- a/src/test/regress/expected/vacuum.out +++ b/src/test/regress/expected/vacuum.out @@ -442,14 +442,20 @@ ALTER TABLE vacowned_parted OWNER TO regress_vacuum; ALTER TABLE vacowned_part1 OWNER TO regress_vacuum; SET ROLE regress_vacuum; VACUUM vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM vacowned_part1; VACUUM vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it ANALYZE vacowned_parted; +WARNING: permission denied to analyze "vacowned_part2", skipping it ANALYZE vacowned_part1; ANALYZE vacowned_part2; +WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; VACUUM (ANALYZE) vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it RESET ROLE; -- Only one partition owned by other user. ALTER TABLE vacowned_parted OWNER TO CURRENT_USER; @@ -478,14 +484,26 @@ ALTER TABLE vacowned_parted OWNER TO regress_vacuum; ALTER TABLE vacowned_part1 OWNER TO CURRENT_USER; SET ROLE regress_vacuum; VACUUM vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part1", skipping it +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM vacowned_part1; +WARNING: permission denied to vacuum "vacowned_part1", skipping it VACUUM vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it ANALYZE vacowned_parted; +WARNING: permission denied to analyze "vacowned_part1", skipping it +WARNING: permission denied to analyze "vacowned_part2", skipping it ANALYZE vacowned_part1; +WARNING: permission denied to analyze "vacowned_part1", skipping it ANALYZE vacowned_part2; +WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; +WARNING: permission denied to vacuum "vacowned_part1", skipping it +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; +WARNING: permission denied to vacuum "vacowned_part1", skipping it VACUUM (ANALYZE) vacowned_part2; +WARNING: permission denied to vacuum "vacowned_part2", skipping it RESET ROLE; DROP TABLE vacowned; DROP TABLE vacowned_parted; diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluster.sql index a4cfaae807..b7115f8610 100644 --- a/src/test/regress/sql/cluster.sql +++ b/src/test/regress/sql/cluster.sql @@ -238,6 +238,9 @@ CREATE TABLE ptnowner1 PARTITION OF ptnowner FOR VALUES IN (1); CREATE ROLE regress_ptnowner; CREATE TABLE ptnowner2 PARTITION OF ptnowner FOR VALUES IN (2); ALTER TABLE ptnowner1 OWNER TO regress_ptnowner; +SET SESSION AUTHORIZATION regress_ptnowner; +CLUSTER ptnowner USING ptnowner_i_idx; +RESET SESSION AUTHORIZATION; ALTER TABLE ptnowner OWNER TO regress_ptnowner; CREATE TEMP TABLE ptnowner_oldnodes AS SELECT oid, relname, relfilenode FROM pg_partition_tree('ptnowner') AS tree -- 2.25.1 --qMm9M+Fa2AknHoGS Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v9-0002-simplify-privilege-related-documentation-for-main.patch" ^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2023-06-19 20:57 UTC | newest] Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-11-26 20:37 [PATCH v6 1/7] pg_dump: make CLUSTER ON a separate dump object.. Justin Pryzby <[email protected]> 2023-06-14 17:54 [PATCH v1 1/2] partial revert of ff9618e82a Nathan Bossart <[email protected]> 2023-06-14 17:54 [PATCH v3 1/2] partial revert of ff9618e82a Nathan Bossart <[email protected]> 2023-06-14 17:54 [PATCH v2 1/2] partial revert of ff9618e82a Nathan Bossart <[email protected]> 2023-06-19 20:57 [PATCH v4 2/3] partial revert of ff9618e82a Nathan Bossart <[email protected]> 2023-06-19 20:57 [PATCH v7 1/2] partial revert of ff9618e82a Nathan Bossart <[email protected]> 2023-06-19 20:57 [PATCH v8 1/2] partial revert of ff9618e82a Nathan Bossart <[email protected]> 2023-06-19 20:57 [PATCH v9 1/2] partial revert of ff9618e82a Nathan Bossart <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox