Hi, On Tue, Jun 30, 2026 at 8:30 AM Nathan Bossart <[email protected]> wrote: > > On Tue, Jun 30, 2026 at 01:47:41PM +0900, Michael Paquier wrote: > > Something that still feels off to me is to blindly use _ext() in > > vacuum_is_permitted_for_relation(), where we *may* already hold a lock > > on the relation whose ACL is checked. In this case missing a relation > > is not fine, so this would make the code more brittle in the > > single-relation case under autovacuum or a VACUUM with a list of > > relations provided by a user. > > Yeah, so we should only use it for get_all_vacuum_rels(), as in the > attached.
Thanks for pointing at commit a556549d7e6d. I spent more time on this, and I wasn't fully aware of what that commit was preventing. The worry I raised earlier turns out to be related: https://www.postgresql.org/message-id/CALj2ACV8NWt0AtBd35km0YTCu7%2BforTjpDm09V3HWJRfGAMhoA%40mail.gmail.com . My general thinking is this: keep unprivileged users from doing more work when possible, especially riskier stuff like locking relations, and perhaps a bunch of comparatively less risky stuff too like memory allocations, acquiring and releasing proc-array lock, starting a transaction, getting a snapshot, committing the transaction, etc. The problem of unnecessary locking of relations by unprivileged users cannot be avoided for vacuum with a table list because it needs to find the OIDs. For autovacuum this isn't a concern, since it runs as a bootstrap superuser and passes OIDs directly, so it clears the privilege check. Removing ACL checks in get_all_vacuum_rels like in v7 effectively brings back the problem that a556549d7e6d fixed. For example, v7 allows something like this to happen: -- create an unprivileged user with no MAINTAIN role create role alice login; -- session 1 begin; select * from pg_authid; -- leave the txn open, holds AccessShareLock on pg_authid until commit -- session 2 -- unprivileged user attempts a database-wide vacuum: set role alice; vacuum full; -- vacuum_rel requests AccessExclusiveLock on pg_authid and gets queued, -- which conflicts with session 1's AccessShareLock, so alice's request -- parks in the queue, waiting. -- This is the strong lock request an unprivileged user should never have -- been able to place at first. -- session 3 -- all new connections would block ./psql -U alice -d postgres So I agree with using the _ext version for the ACL check when building the relations list for database-wide vacuum. It addresses the concurrent table drops issue. The v8 patch looks good to me. Also, I don't have a strong opinion on adding the Assert(missing_ok || CheckRelationOidLockedByMe(relid, AccessShareLock, true)); because the is_missing flag in v8 already conveys whether the caller holds the relation lock or not: missing_ok = true means no lock held, missing_ok = false means the caller holds it. -- Bharath Rupireddy Amazon Web Services: https://aws.amazon.com
