Re: Handle concurrent drop when doing whole database vacuum

Hi,

On Tue, Jun 30, 2026 at 8:30 AM Nathan Bossart <nathandbossart(at)gmail(dot)com>
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