agora inbox for [email protected]  

help / color / mirror / Atom feed

[PATCH 7/8] patched 2
8+ messages / 2 participants
[nested] [flat]

* [PATCH 7/8] patched 2
@ 2021-01-21 23:12  Tomas Vondra <[email protected]>
  0 siblings, 0 replies; 8+ messages in thread

From: Tomas Vondra @ 2021-01-21 23:12 UTC (permalink / raw)

---
 src/backend/access/brin/brin_minmax_multi.c | 284 +++++++++++++-------
 1 file changed, 189 insertions(+), 95 deletions(-)

diff --git a/src/backend/access/brin/brin_minmax_multi.c b/src/backend/access/brin/brin_minmax_multi.c
index 6ef5c5f2bf..094ecd2be9 100644
--- a/src/backend/access/brin/brin_minmax_multi.c
+++ b/src/backend/access/brin/brin_minmax_multi.c
@@ -90,6 +90,7 @@
  */
 #define		PROCNUM_BASE			11
 
+#define		MINMAX_LOAD_FACTOR		0.75
 
 typedef struct MinmaxMultiOpaque
 {
@@ -160,11 +161,11 @@ typedef struct Ranges
 } Ranges;
 
 /*
- * On-disk the summary is stored as a bytea value, represented by the
- * SerializedRanges structure. It has a 4B varlena header, so can treated
- * as varlena directly.
+ * On-disk the summary is stored as a bytea value, with a simple header
+ * with basic metadata, followed by the boundary values. It has a varlena
+ * header, so can be treated as varlena directly.
  *
- * See range_serialize/range_deserialize methods for serialization details.
+ * See range_serialize/range_deserialize for serialization details.
  */
 typedef struct SerializedRanges
 {
@@ -483,6 +484,32 @@ compare_combine_ranges(const void *a, const void *b, void *arg)
 	return 0;
 }
 
+/*
+ * compare_values
+ *	  Compare the values.
+ */
+static int
+compare_values(const void *a, const void *b, void *arg)
+{
+	Datum *da = (Datum *) a;
+	Datum *db = (Datum *) b;
+	Datum r;
+
+	compare_context *cxt = (compare_context *) arg;
+
+	r = FunctionCall2Coll(cxt->cmpFn, cxt->colloid, *da, *db);
+
+	if (DatumGetBool(r))
+		return -1;
+
+	r = FunctionCall2Coll(cxt->cmpFn, cxt->colloid, *db, *da);
+
+	if (DatumGetBool(r))
+		return 1;
+
+	return 0;
+}
+
 /*
  * range_contains_value
  * 		See if the new value is already contained in the range list.
@@ -923,6 +950,8 @@ typedef struct DistanceValue
 
 /*
  * Simple comparator for distance values, comparing the double value.
+ * This is intentionally sorting the distances in descending order, i.e.
+ * the longer gaps will be at the front.
  */
 static int
 compare_distances(const void *a, const void *b)
@@ -931,9 +960,9 @@ compare_distances(const void *a, const void *b)
 	DistanceValue *db = (DistanceValue *)b;
 
 	if (da->value < db->value)
-		return -1;
-	else if (da->value > db->value)
 		return 1;
+	else if (da->value > db->value)
+		return -1;
 
 	return 0;
 }
@@ -942,8 +971,8 @@ compare_distances(const void *a, const void *b)
  * Given an array of combine ranges, compute distance of the gaps betwen
  * the ranges - for ncranges there are (ncranges-1) gaps.
  *
- * We simply call the "distance" function to compute the (min-max) for pairs
- * of consecutive ganges. The function may be fairly expensive, so we do that
+ * We simply call the "distance" function to compute the (max-min) for pairs
+ * of consecutive ranges. The function may be fairly expensive, so we do that
  * just once (and then use it to pick as many ranges to merge as possible).
  *
  * See reduce_combine_ranges for details.
@@ -970,16 +999,20 @@ build_distances(FmgrInfo *distanceFn, Oid colloid,
 		a1 = cranges[i].maxval;
 		a2 = cranges[i+1].minval;
 
-		/* compute length of the empty gap (distance between max/min) */
+		/* compute length of the gap (between max/min) */
 		r = FunctionCall2Coll(distanceFn, colloid, a1, a2);
 
-		/* remember the index */
+		/* remember the index of the gap the distance is for */
 		distances[i].index = i;
 		distances[i].value = DatumGetFloat8(r);
 	}
 
-	/* sort the distances in ascending order */
-	pg_qsort(distances, (ncranges-1), sizeof(DistanceValue), compare_distances);
+	/*
+	 * Sort the distances in descending order, so that the longest gaps
+	 * are at the front.
+	 */
+	pg_qsort(distances, (ncranges-1), sizeof(DistanceValue),
+			 compare_distances);
 
 	return distances;
 }
@@ -1017,6 +1050,7 @@ build_combine_ranges(FmgrInfo *cmp, Oid colloid, Ranges *ranges,
 	return cranges;
 }
 
+#ifdef USE_ASSERT_CHECKING
 /*
  * Counts bondary values needed to store the ranges. Each single-point
  * range is stored using a single value, each regular range needs two.
@@ -1038,10 +1072,26 @@ count_values(CombineRange *cranges, int ncranges)
 
 	return count;
 }
+#endif
 
 /*
- * Combines ranges until the number of boundary values drops below 75%
- * of the capacity (as set by values_per_range reloption).
+ * reduce_combine_ranges
+ *		reduce the ranges until the number of values is low enough
+ *
+ * Combines ranges until the number of boundary values drops below the
+ * threshold specified by max_values. This happens by merging enough
+ * ranges by distance between them.
+ *
+ * Returns the number of result ranges.
+ *
+ * We simply use the global min/max and then add boundaries for enough
+ * largest gaps. Each gap adds 2 values, so we simply use (target/2-1)
+ * distances. Then we simply sort all the values - each two values are
+ * a boundary of a range (possibly collapsed).
+ *
+ * XXX Some of the ranges may be collapsed (i.e. the min/max values are
+ * equal), but we ignore that for now. We could repeat the process,
+ * adding a couple more gaps recursively.
  *
  * XXX The ranges to merge are selected solely using the distance. But
  * that may not be the best strategy, for example when multiple gaps
@@ -1062,66 +1112,86 @@ count_values(CombineRange *cranges, int ncranges)
  * length of the ranges? Or perhaps randomize the choice of ranges, with
  * probability inversely proportional to the distance (the gap lengths
  * may be very close, but not exactly the same).
+ *
+ * XXX Or maybe we could just handle this by using random value as a
+ * tie-break, or by adding random noise to the actual distance.
  */
 static int
 reduce_combine_ranges(CombineRange *cranges, int ncranges,
-					  DistanceValue *distances, int max_values)
+					  DistanceValue *distances, int max_values,
+					  FmgrInfo *cmp, Oid colloid)
 {
 	int i;
+	int		nvalues;
+	Datum  *values;
+
+	compare_context cxt;
+
+	/* total number of gaps between ranges */
 	int	ndistances = (ncranges - 1);
-	int	count = count_values(cranges, ncranges);
+
+	/* number of gaps to keep */
+	int keep = (max_values/2 - 1);
 
 	/*
-	 * We have one fewer 'gaps' than the ranges. We'll be decrementing
-	 * the number of combine ranges (reduction is the primary goal of
-	 * this function), so we must use a separate value.
+	 * Maybe we have sufficiently low number of ranges already?
+	 *
+	 * XXX This should happen before we actually do the expensive stuff
+	 * like sorting, so maybe this should be just an assert.
 	 */
-	for (i = 0; i < ndistances; i++)
-	{
-		int j;
-		int shortest;
+	if (keep >= ndistances)
+		return ncranges;
 
-		if (count <= max_values * 0.75)
-			break;
+	/* sort the values */
+	cxt.colloid = colloid;
+	cxt.cmpFn = cmp;
 
-		shortest = distances[i].index;
+	/* allocate space for the boundary values */
+	nvalues = 0;
+	values = (Datum *) palloc(sizeof(Datum) * max_values);
 
-		/*
-		 * The index must be still valid with respect to the current size
-		 * of cranges array (and it always points to the first range, so
-		 * never to the last one - hence the -1 in the condition).
-		 */
-		Assert(shortest < (ncranges - 1));
+	/* add the global min/max values, from the first/last range */
+	values[nvalues++] = cranges[0].minval;
+	values[nvalues++] = cranges[ncranges-1].maxval;
 
-		if (!cranges[shortest].collapsed && !cranges[shortest+1].collapsed)
-			count -= 2;
-		else if (!cranges[shortest].collapsed || !cranges[shortest+1].collapsed)
-			count -= 1;
+	/* add boundary values for enough gaps */
+	for (i = 0; i < keep; i++)
+	{
+		/* index of the gap between (index) and (index+1) ranges */
+		int index = distances[i].index;
 
-		/*
-		 * Move the values to join the two selected ranges. The new range is
-		 * definiely not collapsed but a regular range.
-		 */
-		cranges[shortest].maxval = cranges[shortest+1].maxval;
-		cranges[shortest].collapsed = false;
+		Assert((index >= 0) && ((index+1) < ncranges));
 
-		/* shuffle the subsequent combine ranges */
-		memmove(&cranges[shortest+1], &cranges[shortest+2],
-				(ncranges - shortest - 2) * sizeof(CombineRange));
+		/* add min from the preceding range, max from the next one */
+		values[nvalues++] = cranges[index].minval;
+		values[nvalues++] = cranges[index+1].minval;
 
-		/* also, shuffle all higher indexes (we've just moved the ranges) */
-		for (j = i; j < ndistances; j++)
-		{
-			if (distances[j].index > shortest)
-				distances[j].index--;
-		}
+		Assert(nvalues <= max_values);
+	}
 
-		ncranges--;
+	/* We should have even number of range values. */
+	Assert(nvalues % 2 == 0);
+
+	/*
+	 * Sort the values using the comparator function, and form ranges
+	 * from the sorted result.
+	 */
+	qsort_arg(values, nvalues, sizeof(Datum),
+			  compare_values, (void *) &cxt);
 
-		Assert(ncranges > 0);
+	/* We have nvalues boundary values, which means nvalues/2 ranges. */
+	for (i = 0; i < (nvalues / 2); i++)
+	{
+		cranges[i].minval = values[2*i];
+		cranges[i].maxval = values[2*i + 1];
+
+		/* if the boundary values are the same, it's a collapsed range */
+		cranges[i].collapsed = (compare_values(&values[2*i],
+											   &values[2*i+1],
+											   &cxt) == 0);
 	}
 
-	return ncranges;
+	return (nvalues / 2);
 }
 
 /*
@@ -1251,43 +1321,58 @@ range_add_value(BrinDesc *bdesc, Oid colloid,
 	AssertArrayOrder(cmpFn, colloid, &ranges->values[ranges->nranges*2],
 					 ranges->nvalues);
 
-	/* and we'll also need the 'distance' procedure */
-	distanceFn = minmax_multi_get_procinfo(bdesc, attno, PROCNUM_DISTANCE);
+	/* OK build the combine ranges */
+	cranges = build_combine_ranges(cmpFn, colloid, ranges, newval, &ncranges);
 
-	/*
-	 * The distanceFn calls (which may internally call e.g. numeric_le) may
-	 * allocate quite a bit of memory, and we must not leak it. Otherwise
-	 * we'd have problems e.g. when building indexes. So we create a local
-	 * memory context and make sure we free the memory before leaving this
-	 * function (not after every call).
-	 */
-	ctx = AllocSetContextCreate(CurrentMemoryContext,
-								"minmax-multi context",
-								ALLOCSET_DEFAULT_SIZES);
+	/* Reduce the ranges if needed */
+	if (ncranges > ranges->maxvalues)
+	{
+		/* and we'll also need the 'distance' procedure */
+		distanceFn = minmax_multi_get_procinfo(bdesc, attno, PROCNUM_DISTANCE);
 
-	oldctx = MemoryContextSwitchTo(ctx);
+		/*
+		 * The distanceFn calls (which may internally call e.g. numeric_le) may
+		 * allocate quite a bit of memory, and we must not leak it. Otherwise
+		 * we'd have problems e.g. when building indexes. So we create a local
+		 * memory context and make sure we free the memory before leaving this
+		 * function (not after every call).
+		 */
+		ctx = AllocSetContextCreate(CurrentMemoryContext,
+									"minmax-multi context",
+									ALLOCSET_DEFAULT_SIZES);
 
-	/* OK build the combine ranges */
-	cranges = build_combine_ranges(cmpFn, colloid, ranges, newval, &ncranges);
+		oldctx = MemoryContextSwitchTo(ctx);
 
-	/* build array of gap distances and sort them in ascending order */
-	distances = build_distances(distanceFn, colloid, cranges, ncranges);
+		/* build array of gap distances and sort them in ascending order */
+		distances = build_distances(distanceFn, colloid, cranges, ncranges);
 
-	/*
-	 * Combine ranges until we release at least 25% of the space. This
-	 * threshold is somewhat arbitrary, perhaps needs tuning. We must not
-	 * use too low or high value.
-	 */
-	ncranges = reduce_combine_ranges(cranges, ncranges, distances,
-									 ranges->maxvalues);
+		/*
+		 * Combine ranges until we release at least 25% of the space. This
+		 * threshold is somewhat arbitrary, perhaps needs tuning. We must not
+		 * use too low or high value.
+		 */
+		ncranges = reduce_combine_ranges(cranges, ncranges, distances,
+										 ranges->maxvalues * MINMAX_LOAD_FACTOR,
+										 cmpFn, colloid);
 
-	Assert(count_values(cranges, ncranges) <= ranges->maxvalues * 0.75);
+		Assert(count_values(cranges, ncranges) <= ranges->maxvalues * MINMAX_LOAD_FACTOR);
 
-	/* decompose the combine ranges into regular ranges and single values */
-	store_combine_ranges(ranges, cranges, ncranges);
+		/* decompose the combine ranges into regular ranges and single values */
+		store_combine_ranges(ranges, cranges, ncranges);
 
-	MemoryContextSwitchTo(oldctx);
-	MemoryContextDelete(ctx);
+		MemoryContextSwitchTo(oldctx);
+		MemoryContextDelete(ctx);
+	}
+	else
+		/*
+		 * This seems like a fairly expensive thing, maybe we could just
+		 * modify the ranges directly, instead of building cranges and
+		 * decomposing them again?
+		 */
+		store_combine_ranges(ranges, cranges, ncranges);
+
+	/* combine ranges were allocated outside the memory context */
+	pfree(cranges);
 
 	/* Check the ordering invariants are not violated (for both parts). */
 	AssertArrayOrder(cmpFn, colloid, ranges->values, ranges->nranges*2);
@@ -1795,7 +1880,7 @@ brin_minmax_multi_add_value(PG_FUNCTION_ARGS)
 
 	/*
 	 * Try to add the new value to the range. We need to update the modified
-	 * flag, so that we serialize the correct value.
+	 * flag, so that we serialize the updated summary later.
 	 */
 	modified |= range_add_value(bdesc, colloid, attno, attr, ranges, newval);
 
@@ -2066,19 +2151,28 @@ brin_minmax_multi_union(PG_FUNCTION_ARGS)
 	/* check that the combine ranges are correct (no overlaps, ordering) */
 	AssertValidCombineRanges(bdesc, colloid, attno, attr, cranges, ncranges);
 
-	/* build array of gap distances and sort them in ascending order */
-	distanceFn = minmax_multi_get_procinfo(bdesc, attno, PROCNUM_DISTANCE);
-	distances = build_distances(distanceFn, colloid, cranges, ncranges);
-
 	/*
-	 * See how many values would be needed to store the current ranges, and if
-	 * needed combine as many off them to get below the maxvalues threshold.
-	 * The collapsed ranges will be stored as a single value.
-	 *
-	 * XXX The maxvalues may be different, so perhaps use Max?
+	 * If needed, reduce some of the ranges. This may be fairly expensive,
+	 * so only do that when necessary (when we have too many ranges).
 	 */
-	ncranges = reduce_combine_ranges(cranges, ncranges, distances,
-									 ranges_a->maxvalues);
+	if (ncranges > ranges_a->maxvalues)
+	{
+		/* build array of gap distances and sort them in ascending order */
+		distanceFn = minmax_multi_get_procinfo(bdesc, attno, PROCNUM_DISTANCE);
+		distances = build_distances(distanceFn, colloid, cranges, ncranges);
+
+		/*
+		 * See how many values would be needed to store the current ranges,
+		 * and if needed combine as many off them to get below the threshold.
+		 * The collapsed ranges will be stored as a single value.
+		 *
+		 * XXX Can the maxvalues be different in the two ranges? Perhaps
+		 * we should use maximum of those?
+		 */
+		ncranges = reduce_combine_ranges(cranges, ncranges, distances,
+										 ranges_a->maxvalues,
+										 cmpFn, colloid);
+	}
 
 	/* update the first range summary */
 	store_combine_ranges(ranges_a, cranges, ncranges);
-- 
2.26.2


--------------FA974B75A0C3E9CA18CE7207
Content-Type: text/x-patch; charset=UTF-8;
 name="0008-batch-build-20210122.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="0008-batch-build-20210122.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 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

* [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

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 --
2021-01-21 23:12 [PATCH 7/8] patched 2 Tomas Vondra <[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 v9 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]>

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox