postgres.git / summary / log / commit / refs
commit 9bb8e16bd53e2c8a822bf13832c4dcb3905a34e5
Author: Álvaro Herrera <alvherre@kurilemu.de>
Date: Wed Aug 19 10:25:06 2026 +0000
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.
While at it, update the comment in RangeVarCallbackMaintainsTable to
also mention REPACK.
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Backpatch-through: 19
Discussion: https://www.postgresql.org/message-id/CALj2ACX3pyuRS8%2B%2B6L20cJUMRTf_qbbVp69J1btJ3y6%3D77e5gw%40mail.gmail.com
src/backend/commands/repack.c | 35 ++++++++++++++++++++++++-----------
src/backend/commands/tablecmds.c | 2 +-
2 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index edff54e734e..477c86b2ba6 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 may have been removed recently. If
+ * it's been removed, silently return false. If the relation exists 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 the relation exists and user has required perms, so 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;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1f2411a33a8..c7fde625279 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -20187,7 +20187,7 @@ AtEOSubXact_on_commit_actions(bool isCommit, SubTransactionId mySubid,
* the relation to be locked only if (1) it's a plain or partitioned table,
* materialized view, or TOAST table and (2) the current user is the owner (or
* the superuser) or has been granted MAINTAIN. This meets the
- * permission-checking needs of CLUSTER, REINDEX TABLE, and REFRESH
+ * permission-checking needs of CLUSTER, REPACK, REINDEX TABLE, and REFRESH
* MATERIALIZED VIEW; we expose it here so that it can be used by all.
*/
void
[parent: abca12838fef]