agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Álvaro 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()
Date: Mon, 17 Aug 2026 17:56:19 +0200
Message-ID: <aoMus7tN9VHnB-Jr@alvherre.pgsql> (raw)
In-Reply-To: <CALj2ACVExk=_m=7nuwVE5Nb3kwsYm1498NSO=u0uOOSWjDCdTQ@mail.gmail.com>
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
Attachments:
[text/x-diff] v3-0001-Tighten-ACL-check-in-repack_is_permitted_for_rela.patch (5.0K, ../aoMus7tN9VHnB-Jr@alvherre.pgsql/2-v3-0001-Tighten-ACL-check-in-repack_is_permitted_for_rela.patch)
download | inline diff:
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
view thread (7+ messages) latest in thread
Message-ID: <aoMus7tN9VHnB-Jr@alvherre.pgsql>
Permalink: ../aoMus7tN9VHnB-Jr@alvherre.pgsql/
Also on: postgresql.org/message-id/aoMus7tN9VHnB-Jr@alvherre.pgsql
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@kurilemu.de, bharath.rupireddyforpostgres@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Tighten ACL check in repack_is_permitted_for_relation()
In-Reply-To: <aoMus7tN9VHnB-Jr@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox