Date: Wed, 14 Dec 2022 16:27:05 -0800
From: Nathan Bossart <nathandbossart@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Jeff Davis <pgsql@j-davis.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Pavel Luzanov <p.luzanov@postgrespro.ru>,
	pgsql-hackers@postgresql.org
Subject: Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
Message-ID: <20221215002705.GA889413@nathanxps13>
References: <20221214171336.GB689106@nathanxps13>
 <20221214180534.jglo7jclow3x73bd@alvherre.pgsql>
 <20221214181659.GB773264@nathanxps13>
 <efd049fb7407c5eb0cc3212e7ea43bebf48eab0f.camel@j-davis.com>
 <20221214232939.GA828727@nathanxps13>
 <Y5pmaksAYyJbSU77@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="ibTvN161/egqYuK8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <Y5pmaksAYyJbSU77@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/20221215002705.GA889413%40nathanxps13>
Precedence: bulk


--ibTvN161/egqYuK8
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On Thu, Dec 15, 2022 at 09:12:26AM +0900, Michael Paquier wrote:
> On Wed, Dec 14, 2022 at 03:29:39PM -0800, Nathan Bossart wrote:
>> On Wed, Dec 14, 2022 at 11:05:13AM -0800, Jeff Davis wrote:
>>> On Wed, 2022-12-14 at 10:16 -0800, Nathan Bossart wrote:
>>>> Okay.  Should all the privileges governed by MAINTAIN apply to a
>>>> relation's
>>>> TOAST table as well?
>>> 
>>> Yes, I agree.
>> 
>> This might be tricky, because AFAICT you have to scan pg_class to find a
>> TOAST table's main relation.
> 
> Ugh, yeah.  Are we talking about a case where we know the toast
> information but need to look back at some information of its parent to
> do a decision?  I don't recall a case where we do that.  CLUSTER,
> REINDEX and VACUUM lock first the parent when working on it, and no
> AEL is taken on the parent if doing directly a VACUUM or a REINDEX on
> the toast table, so that could lead to deadlock scenarios.  Shouldn't
> MAINTAIN be sent down to the toast table as well if that's not done
> this way?

Another option I'm looking at is skipping the privilege checks when VACUUM
recurses to a TOAST table.  This won't allow you to VACUUM the TOAST table
directly, but it would at least address the originally-reported issue [0].

Since you can't ANALYZE, REFRESH, or LOCK TOAST tables, this isn't a
problem for those commands.  CLUSTER and REINDEX seem to process relations'
TOAST tables without extra privilege checks already.  So with the attached
patch applied, you wouldn't be able to VACUUM, CLUSTER, and REINDEX TOAST
tableѕ directly (unless you were given MAINTAIN or pg_maintain), but you
could indirectly process them by specifying the main relation.

I don't know if this is good enough.  It seems like ideally you should be
able to VACUUM a TOAST table directly if you have MAINTAIN on its main
relation.

[0] https://postgr.es/m/b572d238-0de2-9cad-5f34-4741dc627834%40postgrespro.ru

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

--ibTvN161/egqYuK8
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="allow_vacuuming_toast.patch"

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 293b84bbca..8f80e5d8c0 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -91,7 +91,8 @@ static void vac_truncate_clog(TransactionId frozenXID,
 							  MultiXactId minMulti,
 							  TransactionId lastSaneFrozenXid,
 							  MultiXactId lastSaneMinMulti);
-static bool vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params);
+static bool vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
+					   bool skip_privs);
 static double compute_parallel_delay(void);
 static VacOptValue get_vacoptval_from_boolean(DefElem *def);
 static bool vac_tid_reaped(ItemPointer itemptr, void *state);
@@ -470,7 +471,7 @@ vacuum(List *relations, VacuumParams *params,
 
 			if (params->options & VACOPT_VACUUM)
 			{
-				if (!vacuum_rel(vrel->oid, vrel->relation, params))
+				if (!vacuum_rel(vrel->oid, vrel->relation, params, false))
 					continue;
 			}
 
@@ -1806,7 +1807,7 @@ vac_truncate_clog(TransactionId frozenXID,
  *		At entry and exit, we are not inside a transaction.
  */
 static bool
-vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
+vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params, bool skip_privs)
 {
 	LOCKMODE	lmode;
 	Relation	rel;
@@ -1893,7 +1894,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	 * happen across multiple transactions where privileges could have changed
 	 * in-between.  Make sure to only generate logs for VACUUM in this case.
 	 */
-	if (!vacuum_is_permitted_for_relation(RelationGetRelid(rel),
+	if (!skip_privs &&
+		!vacuum_is_permitted_for_relation(RelationGetRelid(rel),
 										  rel->rd_rel,
 										  params->options & VACOPT_VACUUM))
 	{
@@ -2067,7 +2069,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	 * totally unimportant for toast relations.
 	 */
 	if (toast_relid != InvalidOid)
-		vacuum_rel(toast_relid, NULL, params);
+		vacuum_rel(toast_relid, NULL, params, true);
 
 	/*
 	 * Now release the session-level lock on the main table.
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 34c26a804a..5335e45434 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -2901,6 +2901,7 @@ CREATE INDEX ON maintain_test (a);
 GRANT MAINTAIN ON maintain_test TO regress_maintain;
 CREATE MATERIALIZED VIEW refresh_test AS SELECT 1;
 GRANT MAINTAIN ON refresh_test TO regress_maintain;
+GRANT MAINTAIN ON pg_type TO regress_maintain;
 CREATE SCHEMA reindex_test;
 -- negative tests; should fail
 SET ROLE regress_no_maintain;
@@ -2910,6 +2911,8 @@ ANALYZE maintain_test;
 WARNING:  permission denied to analyze "maintain_test", skipping it
 VACUUM (ANALYZE) maintain_test;
 WARNING:  permission denied to vacuum "maintain_test", skipping it
+VACUUM pg_type;
+WARNING:  permission denied to vacuum "pg_type", skipping it
 CLUSTER maintain_test USING maintain_test_a_idx;
 ERROR:  must be owner of table maintain_test
 REFRESH MATERIALIZED VIEW refresh_test;
@@ -2933,6 +2936,7 @@ SET ROLE regress_maintain;
 VACUUM maintain_test;
 ANALYZE maintain_test;
 VACUUM (ANALYZE) maintain_test;
+VACUUM pg_type;
 CLUSTER maintain_test USING maintain_test_a_idx;
 REFRESH MATERIALIZED VIEW refresh_test;
 REINDEX TABLE maintain_test;
@@ -2950,6 +2954,7 @@ SET ROLE regress_maintain_all;
 VACUUM maintain_test;
 ANALYZE maintain_test;
 VACUUM (ANALYZE) maintain_test;
+VACUUM pg_type;
 CLUSTER maintain_test USING maintain_test_a_idx;
 REFRESH MATERIALIZED VIEW refresh_test;
 REINDEX TABLE maintain_test;
@@ -2965,6 +2970,7 @@ RESET ROLE;
 DROP TABLE maintain_test;
 DROP MATERIALIZED VIEW refresh_test;
 DROP SCHEMA reindex_test;
+REVOKE MAINTAIN ON pg_type FROM regress_maintain;
 DROP ROLE regress_no_maintain;
 DROP ROLE regress_maintain;
 DROP ROLE regress_maintain_all;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 39aa0b4ecf..5f36026eaa 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -1872,6 +1872,7 @@ CREATE INDEX ON maintain_test (a);
 GRANT MAINTAIN ON maintain_test TO regress_maintain;
 CREATE MATERIALIZED VIEW refresh_test AS SELECT 1;
 GRANT MAINTAIN ON refresh_test TO regress_maintain;
+GRANT MAINTAIN ON pg_type TO regress_maintain;
 CREATE SCHEMA reindex_test;
 
 -- negative tests; should fail
@@ -1879,6 +1880,7 @@ SET ROLE regress_no_maintain;
 VACUUM maintain_test;
 ANALYZE maintain_test;
 VACUUM (ANALYZE) maintain_test;
+VACUUM pg_type;
 CLUSTER maintain_test USING maintain_test_a_idx;
 REFRESH MATERIALIZED VIEW refresh_test;
 REINDEX TABLE maintain_test;
@@ -1896,6 +1898,7 @@ SET ROLE regress_maintain;
 VACUUM maintain_test;
 ANALYZE maintain_test;
 VACUUM (ANALYZE) maintain_test;
+VACUUM pg_type;
 CLUSTER maintain_test USING maintain_test_a_idx;
 REFRESH MATERIALIZED VIEW refresh_test;
 REINDEX TABLE maintain_test;
@@ -1913,6 +1916,7 @@ SET ROLE regress_maintain_all;
 VACUUM maintain_test;
 ANALYZE maintain_test;
 VACUUM (ANALYZE) maintain_test;
+VACUUM pg_type;
 CLUSTER maintain_test USING maintain_test_a_idx;
 REFRESH MATERIALIZED VIEW refresh_test;
 REINDEX TABLE maintain_test;
@@ -1929,6 +1933,7 @@ RESET ROLE;
 DROP TABLE maintain_test;
 DROP MATERIALIZED VIEW refresh_test;
 DROP SCHEMA reindex_test;
+REVOKE MAINTAIN ON pg_type FROM regress_maintain;
 DROP ROLE regress_no_maintain;
 DROP ROLE regress_maintain;
 DROP ROLE regress_maintain_all;

--ibTvN161/egqYuK8--