From 6729b2f14ed623a8b38f5a530b8c85c96bfd5b40 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy Date: Fri, 10 Jul 2026 15:42:47 +0000 Subject: [PATCH v1] Tighten ACL check in repack_is_permitted_for_relation() Commit c71d43025d7 made repack_is_permitted_for_relation() use _ext() and skip a concurrently dropped relation silently. That is fine for the callers that hold no lock on the relation, but it is applied unconditionally, including where we may already hold a lock on the relation whose ACL is checked. In that case missing a relation is not fine, so this made the code more brittle: a missing relation there would indicate a bug and should error out rather than be skipped silently. Add a missing_ok flag so the silent skip is done only where no lock is held. Callers that hold a lock pass false and still error out on a missing relation. --- src/backend/commands/repack.c | 38 ++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index 02883fe34a4..b09be65da3b 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -172,8 +172,8 @@ static List *get_tables_to_repack(RepackCommand cmd, bool usingindex, static List *get_tables_to_repack_partitioned(RepackCommand cmd, Oid relid, bool rel_is_index, MemoryContext permcxt); -static bool repack_is_permitted_for_relation(RepackCommand cmd, - Oid relid, Oid userid); +static bool repack_is_permitted_for_relation(RepackCommand cmd, Oid relid, + Oid userid, bool missing_ok); static void apply_concurrent_changes(BufFile *file, ChangeContext *chgcxt); static void apply_concurrent_insert(Relation rel, TupleTableSlot *slot, @@ -724,7 +724,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, false)) { relation_close(OldHeap, lmode); return false; @@ -2198,7 +2198,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(), true)) continue; /* Use a permanent memory context for the result list */ @@ -2235,7 +2235,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(), true)) continue; /* Use a permanent memory context for the result list */ @@ -2304,7 +2304,7 @@ get_tables_to_repack_partitioned(RepackCommand cmd, Oid relid, * leaf partition despite having them on the partitioned table. Skip * if so. */ - if (!repack_is_permitted_for_relation(cmd, table_oid, GetUserId())) + if (!repack_is_permitted_for_relation(cmd, table_oid, GetUserId(), true)) continue; /* Use a permanent memory context for the result list */ @@ -2321,16 +2321,21 @@ get_tables_to_repack_partitioned(RepackCommand cmd, Oid relid, /* - * Return whether userid has privileges to execute REPACK on relid. + * Check whether the passed-in user has privileges to execute REPACK on the + * relation. * - * Caller may not have a lock on the relation, so it could have been - * dropped concurrently. In that case, silently return false. + * missing_ok should be passed only by callers that hold no lock on the + * relation, where a concurrent drop is possible and fine to skip. A caller + * that holds a lock must not pass it: there a missing relation would indicate + * a bug and should error out rather than be silently skipped. * * If the relation does exist but the user doesn't have the required - * privs, emit a WARNING and return false. Otherwise, return true. + * privileges, emit a WARNING and return false to let the caller + * decide what to do with this relation. */ 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 missing_ok) { bool is_missing = false; AclResult result; @@ -2338,9 +2343,18 @@ repack_is_permitted_for_relation(RepackCommand cmd, Oid relid, Oid userid) Assert(cmd == REPACK_COMMAND_CLUSTER || cmd == REPACK_COMMAND_REPACK); - result = pg_class_aclcheck_ext(relid, userid, ACL_MAINTAIN, &is_missing); + result = pg_class_aclcheck_ext(relid, userid, ACL_MAINTAIN, + missing_ok ? &is_missing : NULL); + + /* + * If the relation was concurrently dropped, nothing to do. Note that this + * is only reachable when the caller specified missing_ok. + */ if (is_missing) + { + Assert(missing_ok); return false; + } if (result == ACLCHECK_OK) return true; -- 2.47.3