agora inbox for pgsql-hackers@postgresql.orghelp / color / mirror / Atom feed
[PATCH v3] Tighten ACL check in repack_is_permitted_for_relation() 1+ messages / 1 participants [nested] [flat]
* [PATCH v3] Tighten ACL check in repack_is_permitted_for_relation() @ 2026-08-17 15:53 Álvaro Herrera <alvherre@kurilemu.de> 0 siblings, 0 replies; 1+ messages in thread From: Álvaro Herrera @ 2026-08-17 15:53 UTC (permalink / raw) 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%40ma... --- 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-- ^ permalink raw reply [nested|flat] 1+ messages in thread
only message in thread Thread overview: 1+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-08-17 15:53 [PATCH v3] Tighten ACL check in repack_is_permitted_for_relation() Álvaro Herrera <alvherre@kurilemu.de>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox