Feedback-ID: ie3de48e3:Fastmail
Date: Mon, 17 Aug 2026 17:56:19 +0200
From: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Tighten ACL check in repack_is_permitted_for_relation()
Message-ID: <aoMus7tN9VHnB-Jr@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="lagmlhwoe2cxax2o"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CALj2ACVExk=_m=7nuwVE5Nb3kwsYm1498NSO=u0uOOSWjDCdTQ@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/aoMus7tN9VHnB-Jr%40alvherre.pgsql>
Precedence: bulk


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

Hello

On 2026-Aug-04, Bharath Rupireddy wrote:

> repack_is_permitted_for_relation() uses pg_class_aclcheck_ext() to
> silently skip a concurrently-dropped relation. That's wrong for a
> caller that may already hold a lock on the relation whose ACL is
> checked, where missing a relation is not fine, and it makes the
> single-relation REPACK cases more brittle
> (https://www.postgresql.org/message-id/akPhEffRipH4isWF@nathan). So
> only detect a missing relation where that's expected, following the
> fix for vacuum_is_permitted_for_relation() in commit 824d5f6.

That makes sense.  I think "missing OK" is a bit weird as an argument
here though; I prefer it as "already locked", inverting the boolean.
What do you think of this formulation?

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Learn about compilers. Then everything looks like either a compiler or
a database, and now you have two problems but one of them is fun."
            https://twitter.com/thingskatedid/status/1456027786158776329

--lagmlhwoe2cxax2o
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename=v3-0001-Tighten-ACL-check-in-repack_is_permitted_for_rela.patch

From 7efe46210cd9e5c943f02c2a55757f23b2bfec9a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Mon, 17 Aug 2026 17:53:34 +0200
Subject: [PATCH v3] Tighten ACL check in repack_is_permitted_for_relation()

repack_is_permitted_for_relation() uses pg_class_aclcheck_ext()
to silently skip a concurrently-dropped relation.  That's wrong
for a caller that may already hold a lock on the relation whose
ACL is checked, where missing a relation is not fine, and it
makes the single-relation REPACK and CLUSTER cases more brittle.
So only detect a missing relation where that's expected,
following the fix for vacuum_is_permitted_for_relation() in
commit 824d5f6241ea.

The new already_locked behavior is limited to get_tables_to_repack()
and get_tables_to_repack_partitioned().  All other callers of
repack_is_permitted_for_relation() hold a lock on the relation
that prevents it from being concurrently dropped, so this commit
also adds an assertion to that effect.

Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Discussion: https://www.postgresql.org/message-id/CALj2ACX3pyuRS8%2B%2B6L20cJUMRTf_qbbVp69J1btJ3y6%3D77e5gw%40mail.gmail.com
---
 src/backend/commands/repack.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index edff54e734e..66b88e28c2e 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -173,7 +173,8 @@ static List *get_tables_to_repack_partitioned(RepackStmt *stmt,
 											  Relation rel,
 											  MemoryContext permcxt);
 static bool repack_is_permitted_for_relation(RepackCommand cmd,
-											 Oid relid, Oid userid);
+											 Oid relid, Oid userid,
+											 bool already_locked);
 
 static void apply_concurrent_changes(BufFile *file, ChangeContext *chgcxt);
 static void apply_concurrent_insert(Relation rel, TupleTableSlot *slot,
@@ -681,7 +682,7 @@ cluster_rel_recheck(RepackCommand cmd, Relation OldHeap, Oid indexOid,
 	Assert(CheckRelationLockedByMe(OldHeap, lmode, false));
 
 	/* Check that the user still has privileges for the relation */
-	if (!repack_is_permitted_for_relation(cmd, tableOid, userid))
+	if (!repack_is_permitted_for_relation(cmd, tableOid, userid, true))
 	{
 		relation_close(OldHeap, lmode);
 		return false;
@@ -2155,7 +2156,7 @@ get_tables_to_repack(RepackCommand cmd, bool usingindex, MemoryContext permcxt)
 
 			/* noisily skip rels which the user can't process */
 			if (!repack_is_permitted_for_relation(cmd, index->indrelid,
-												  GetUserId()))
+												  GetUserId(), false))
 				continue;
 
 			/* Use a permanent memory context for the result list */
@@ -2192,7 +2193,7 @@ get_tables_to_repack(RepackCommand cmd, bool usingindex, MemoryContext permcxt)
 
 			/* noisily skip rels which the user can't process */
 			if (!repack_is_permitted_for_relation(cmd, class->oid,
-												  GetUserId()))
+												  GetUserId(), false))
 				continue;
 
 			/* Use a permanent memory context for the result list */
@@ -2321,7 +2322,7 @@ get_tables_to_repack_partitioned(RepackStmt *stmt, Relation rel,
 		 * if so.
 		 */
 		if (!repack_is_permitted_for_relation(stmt->command, table_oid,
-											  GetUserId()))
+											  GetUserId(), false))
 			continue;
 
 		/* Use a permanent memory context for the result list */
@@ -2341,26 +2342,38 @@ get_tables_to_repack_partitioned(RepackStmt *stmt, Relation rel,
 
 
 /*
- * Return whether userid has privileges to execute REPACK on relid.
+ * Return whether userid has privileges to execute REPACK/CLUSTER on relid.
  *
- * Caller may not have a lock on the relation, so it could have been
- * dropped concurrently.  In that case, silently return false.
+ * The relation may already be locked by caller, in which case it cannot
+ * possibly go missing; otherwise it can have been removed recently.  If
+ * it's been removed, silently return false.  If the relation does exist but
+ * the user doesn't have the required privs, emit a WARNING and return false.
  *
- * If the relation does exist but the user doesn't have the required
- * privs, emit a WARNING and return false.  Otherwise, return true.
+ * Otherwise, return true.
  */
 static bool
-repack_is_permitted_for_relation(RepackCommand cmd, Oid relid, Oid userid)
+repack_is_permitted_for_relation(RepackCommand cmd, Oid relid, Oid userid,
+								 bool already_locked)
 {
 	bool		is_missing = false;
 	AclResult	result;
 	char	   *relname;
 
 	Assert(cmd == REPACK_COMMAND_CLUSTER || cmd == REPACK_COMMAND_REPACK);
+	Assert(!already_locked ||
+		   CheckRelationOidLockedByMe(relid, AccessShareLock, true));
 
 	result = pg_class_aclcheck_ext(relid, userid, ACL_MAINTAIN, &is_missing);
+
+	/*
+	 * If the relation was concurrently dropped, nothing to do.  This is only
+	 * reachable when the caller doesn't already have a lock on the relation.
+	 */
 	if (is_missing)
+	{
+		Assert(!already_locked);
 		return false;
+	}
 
 	if (result == ACLCHECK_OK)
 		return true;
-- 
2.47.3


--lagmlhwoe2cxax2o--