Date: Tue, 20 Jun 2023 10:40:32 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Jeff Davis <pgsql@j-davis.com>
Cc: Michael Paquier <michael@paquier.xyz>, Ted Yu <yuzhihong@gmail.com>,
	Pavel Luzanov <p.luzanov@postgrespro.ru>,
	Justin Pryzby <pryzby@telsasoft.com>, pgsql-hackers@postgresql.org
Subject: Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
Message-ID: <20230620174032.GA471329@nathanxps13>
References: <20230613211246.GA219055@nathanxps13>
 <ZIj4v1CwqlDVJZfB@paquier.xyz>
 <20230613235442.GA222795@nathanxps13>
 <20230614181711.GA488295@nathanxps13>
 <ZIpfack6qJUHaw+g@paquier.xyz>
 <20230615041044.GA736001@nathanxps13>
 <20230615235700.GA877311@nathanxps13>
 <20230616052025.GA1026700@nathanxps13>
 <20230619215534.GA442477@nathanxps13>
 <dd53c632abf86fc6dacf6653f9fbab9110ccdc14.camel@j-davis.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="h31gzZEtNLTqOjlF"
Content-Disposition: inline
In-Reply-To: <dd53c632abf86fc6dacf6653f9fbab9110ccdc14.camel@j-davis.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20230620174032.GA471329%40nathanxps13>
Precedence: bulk


--h31gzZEtNLTqOjlF
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Jun 20, 2023 at 10:04:37AM -0700, Jeff Davis wrote:
> I think v4-0001 broke the handling of toast tables? It looks like you
> removed the check for !skip_privs but need to add it to the flags in
> vacuum_is_permitted_for_relation(). 

Good catch.  I'm not sure why some of the calls to
vacuum_is_permitted_for_relation() are masking the options.  AFAICT we can
simply remove the masks.  I've done so in the attached patch.

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

--h31gzZEtNLTqOjlF
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v5-0001-convert-boolean-parameter-into-a-VACOPT_-option.patch"

From b7528138025f6fae062008fe4e166957789bdc25 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Wed, 14 Jun 2023 11:08:03 -0700
Subject: [PATCH v5 1/1] convert boolean parameter into a VACOPT_* option

---
 src/backend/commands/analyze.c |  2 +-
 src/backend/commands/vacuum.c  | 28 +++++++++++++++++++---------
 src/include/commands/vacuum.h  |  1 +
 3 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 52ef462dba..08f7ba1623 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -167,7 +167,7 @@ analyze_rel(Oid relid, RangeVar *relation,
 	 */
 	if (!vacuum_is_permitted_for_relation(RelationGetRelid(onerel),
 										  onerel->rd_rel,
-										  params->options & VACOPT_ANALYZE))
+										  params->options))
 	{
 		relation_close(onerel, ShareUpdateExclusiveLock);
 		return;
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index a843f9ad92..108b5640d1 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -115,7 +115,7 @@ static void vac_truncate_clog(TransactionId frozenXID,
 							  TransactionId lastSaneFrozenXid,
 							  MultiXactId lastSaneMinMulti);
 static bool vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
-					   bool skip_privs, BufferAccessStrategy bstrategy);
+					   BufferAccessStrategy bstrategy);
 static double compute_parallel_delay(void);
 static VacOptValue get_vacoptval_from_boolean(DefElem *def);
 static bool vac_tid_reaped(ItemPointer itemptr, void *state);
@@ -620,8 +620,7 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy,
 
 			if (params->options & VACOPT_VACUUM)
 			{
-				if (!vacuum_rel(vrel->oid, vrel->relation, params, false,
-								bstrategy))
+				if (!vacuum_rel(vrel->oid, vrel->relation, params, bstrategy))
 					continue;
 			}
 
@@ -712,6 +711,13 @@ vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple,
 
 	Assert((options & (VACOPT_VACUUM | VACOPT_ANALYZE)) != 0);
 
+	/*
+	 * Privilege checks are bypassed in some cases (e.g., when recursing to a
+	 * relation's TOAST table).
+	 */
+	if (options & VACOPT_SKIP_PRIVS)
+		return true;
+
 	/*----------
 	 * A role has privileges to vacuum or analyze the relation if any of the
 	 * following are true:
@@ -1953,7 +1959,7 @@ vac_truncate_clog(TransactionId frozenXID,
  */
 static bool
 vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
-		   bool skip_privs, BufferAccessStrategy bstrategy)
+		   BufferAccessStrategy bstrategy)
 {
 	LOCKMODE	lmode;
 	Relation	rel;
@@ -2040,10 +2046,9 @@ 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 (!skip_privs &&
-		!vacuum_is_permitted_for_relation(RelationGetRelid(rel),
+	if (!vacuum_is_permitted_for_relation(RelationGetRelid(rel),
 										  rel->rd_rel,
-										  params->options & VACOPT_VACUUM))
+										  params->options))
 	{
 		relation_close(rel, lmode);
 		PopActiveSnapshot();
@@ -2229,11 +2234,16 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 	{
 		VacuumParams toast_vacuum_params;
 
-		/* force VACOPT_PROCESS_MAIN so vacuum_rel() processes it */
+		/*
+		 * Force VACOPT_PROCESS_MAIN so vacuum_rel() processes it.  Likewise,
+		 * set VACOPT_SKIP_PRIVS since privileges on the main relation are
+		 * sufficient to process it.
+		 */
 		memcpy(&toast_vacuum_params, params, sizeof(VacuumParams));
 		toast_vacuum_params.options |= VACOPT_PROCESS_MAIN;
+		toast_vacuum_params.options |= VACOPT_SKIP_PRIVS;
 
-		vacuum_rel(toast_relid, NULL, &toast_vacuum_params, true, bstrategy);
+		vacuum_rel(toast_relid, NULL, &toast_vacuum_params, bstrategy);
 	}
 
 	/*
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 17e9b4f68e..cb5b11ab31 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -191,6 +191,7 @@ typedef struct VacAttrStats
 #define VACOPT_DISABLE_PAGE_SKIPPING 0x100	/* don't skip any pages */
 #define VACOPT_SKIP_DATABASE_STATS 0x200	/* skip vac_update_datfrozenxid() */
 #define VACOPT_ONLY_DATABASE_STATS 0x400	/* only vac_update_datfrozenxid() */
+#define VACOPT_SKIP_PRIVS 0x800 /* skip privilege checks */
 
 /*
  * Values used by index_cleanup and truncate params.
-- 
2.25.1


--h31gzZEtNLTqOjlF--