Handle concurrent drop when doing whole database vacuum

Hello.

At Sun, 14 Jun 2026 15:12:43 +0800, "cca5507" <cca5507@qq.com> wrote in

When doing a whole database vacuum, we scan pg_class to construct
a list of vacuumable tables. For each vacuumable table, we call
vacuum_is_permitted_for_relation() to check permissions. If a
concurrent drop happens, the pg_class_aclcheck() might report an
error because of failing to search the syscache:

ERROR: relation with OID ****** does not exist

Good catch!

To fix it, we can use pg_class_aclcheck_ext() to detect the concurrent
drop and report a warning instead.

Another possible direction might be to take a lock when building the
list, instead of dealing with the race later. For example, the repack
command seems to take a light lock when it finds a candidate relation:

static List *
get_tables_to_repack(RepackCommand cmd, bool usingindex, MemoryContext permcxt)

/*
* Try to obtain a light lock on the table, to ensure it doesn't
* go away while we collect the list. If we cannot, just
* disregard the table. Be sure to release this if we ultimately
* decide not to process the table!
*/
if (!ConditionalLockRelationOid(class->oid, AccessShareLock))
continue;

Whether a relation that disappears immediately after being added to
the list should be processed or skipped does not seem particularly
important in practice. However, taking a lock at list construction
time may make the subsequent processing simpler. I wonder whether that
would be a reasonable direction for VACUUM as well.

Regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hi Kyotaro,

 To fix it, we can use pg_class_aclcheck_ext() to detect the concurrent
 drop and report a warning instead.

Another possible direction might be to take a lock when building the
list, instead of dealing with the race later. For example, the repack
command seems to take a light lock when it finds a candidate relation:

static List *
get_tables_to_repack(RepackCommand cmd, bool usingindex, MemoryContext permcxt)

/*
 * Try to obtain a light lock on the table, to ensure it doesn't
 * go away while we collect the list.  If we cannot, just
 * disregard the table.  Be sure to release this if we ultimately
 * decide not to process the table!
 */
if (!ConditionalLockRelationOid(class->oid, AccessShareLock))
continue;

Whether a relation that disappears immediately after being added to
the list should be processed or skipped does not seem particularly
important in practice. However, taking a lock at list construction
time may make the subsequent processing simpler. I wonder whether that
would be a reasonable direction for VACUUM as well.

I don't think it's a good idea because there might be a lot of tables need
to be locked. And a concurrent drop can always happen after the list
construction because we process each table in a separate transaction.
So I think there is no need to prevent concurrent drops at list construction
time. I also prepare to write a patch to remove the lock in get_tables_to_repack().

--
Regards,
ChangAo Chen

Hi ChangAo,

Thank you for reporting and fixing the issue.
The race condition looks real, I confirmed it against the current HEAD.
One thing worth adding to the diagnosis: list construction in
get_all_vacuum_rels() runs in the outer transaction, before vacuum() enters
its PG_TRY block,
so the error from pg_class_aclmask_ext() will abort the entire VACUUM
operation. This is clearly a bug

Updating vacuum_is_permitted_for_relation() to call pg_class_aclcheck_ext()
with is_missing rather than pg_class_aclcheck() looks right to me.
For pg_class_aclcheck(), I checked the other three callers,
expand_vacuum_rel(), vacuum_rel(), and analyze_rel() and each one already
holds a relation lock by the time it reaches this function,
and the error will never fire in those paths.

On Kyotaro's alternative of taking ConditionalLockRelationOid() during list
construction, in the style of get_tables_to_repack().
I agree holding many locks during list build is a real cost on busy
databases, and since each table is processed in its
own transaction, vacuum_open_relation() still has to handle the post-list
drop case regardless.

A couple of points on the patch itself:
1. The bug is racy but the injection_points framework
(src/test/modules/injection_points) can make it deterministic.
We can put an INJECTION_POINT() inside the heap_getnext() loop in
get_all_vacuum_rels() and adding an isolation spec that parks
VACUUM there, runs DROP TABLE in another session, then resumes VACUUM and
asserts it completes with a WARNING.

2. Minor comment vacuum_open_relation() already emits an identically-worded
"relation no longer exists" message with errcode(ERRCODE_UNDEFINED_TABLE).
Worth adding the same errcode to the two new ereports so the SQLSTATE stays
consistent for the same logical event.

This looks like a long-standing bug and I feel it should be backported.

Overall, the patch is heading in the right direction.

Regards,
Surya Poondla

Hi Surya,

Thanks for the comments!

A couple of points on the patch itself:
1. The bug is racy but the injection_points framework (src/test/modules/injection_points) can make it deterministic.
We can put an INJECTION_POINT() inside the heap_getnext() loop in get_all_vacuum_rels() and adding an isolation spec that parks
VACUUM there, runs DROP TABLE in another session, then resumes VACUUM and asserts it completes with a WARNING.

2. Minor comment vacuum_open_relation() already emits an identically-worded
"relation no longer exists" message with errcode(ERRCODE_UNDEFINED_TABLE).
Worth adding the same errcode to the two new ereports so the SQLSTATE stays consistent for the same logical event.

Fixed. Please see the v2 patches.

--
Regards,
ChangAo Chen

Hi ChangAo,

Thank you for adding the test case.
I tested v2 locally and looks good.

A few suggestions:
In vacuum.c:
1) The header comment on vacuum_is_permitted_for_relation() still describes
only the permission-check path.
Now that the function also signals "relation no longer exists", the header
should mention both cases.
2) A one-line comment at the pg_class_aclcheck_ext() call explaining why
the _ext variant is needed (concurrent drop in the whole-database
list-construction path) would help future readers, and guard against a
cleanup patch that silently reverts to pg_class_aclcheck().

In vacuum_concurrent_drop.spec:
1) The spec sets "client_min_messages = ERROR", which suppresses the very
WARNING the patch emits.
Let's have the client_min_messages to WARNING, so we see the warning in the
.out file.
2) Minor comment, maybe adding the ANALYZE permutation (or a sibling spec)
would test that code path too. (Not a mandatory thing but a good to have)

Regards,
Surya Poondla

Hi Surya,

A few suggestions:
In vacuum.c:
1) The header comment on vacuum_is_permitted_for_relation() still describes only the permission-check path.  
Now that the function also signals "relation no longer exists", the header should mention both cases.
2) A one-line comment at the pg_class_aclcheck_ext() call explaining why the _ext variant is needed (concurrent drop in the whole-database
list-construction path) would help future readers, and guard against a cleanup patch that silently reverts to pg_class_aclcheck().

Fixed.

In vacuum_concurrent_drop.spec:
1) The spec sets "client_min_messages = ERROR", which suppresses the very WARNING the patch emits.
Let's have the client_min_messages to WARNING, so we see the warning in the .out file.

Fixed. We need to grant pg_maintain to regress_vacuum to avoid "permission denied ..."
warnings. For versions (< 17) which don't have pg_maintain role, we may still want to use
"client_min_messages = ERROR".

2) Minor comment, maybe adding the ANALYZE permutation (or a sibling spec) would test that code path too. (Not a mandatory thing but a good to have)

Fixed.

--
Regards,
ChangAo Chen

Hi,

On Sun, Jun 14, 2026 at 12:13 AM cca5507 <cca5507@qq.com> wrote:

Hi hackers,

When doing a whole database vacuum, we scan pg_class to construct
a list of vacuumable tables. For each vacuumable table, we call
vacuum_is_permitted_for_relation() to check permissions. If a
concurrent drop happens, the pg_class_aclcheck() might report an
error because of failing to search the syscache:

ERROR: relation with OID ****** does not exist

To fix it, we can use pg_class_aclcheck_ext() to detect the concurrent
drop and report a warning instead.

Note that a concurrent drop after constructing the list of vacuumable
tables is handled by vacuum_open_relation().

Thoughts?

Nice catch!

IMHO, the vacuum command failing in this situation may not be a huge
problem in practice, because the user sees the error and can re-issue
the command.

get_all_vacuum_rels is called only for database-wide vacuum (neither
autovacuum nor vacuum <<tables-list>> calls it - in those cases, the
permission check happens after the table lock is taken in
vacuum_open_relation).

Let's think about what happens if we don't fix this. For example, a
database-wide vacuum with 1000 tables has scanned 990 of them in
pg_class and the 991st table gets concurrently dropped - the vacuum
command fails and the user needs to notice that and re-issue the
command, at which point it rescans pg_class. What exactly is the
problem we're solving? Is it the vacuum command failing and the user
missing it or forgetting to re-issue? The pg_class scan being
expensive? Or consistency with how autovacuum and vacuum
<<tables-list>> handle this?

ReindexMultipleTables has a similar scan on pg_class with a permission
check, but it is not affected because the permission check is gated
behind shared relations (so droppable user tables never reach it).

Some comments on the v3 patches:

1/
+ *
+ * Note: use pg_class_aclcheck_ext() to detect a concurrent drop.

This comment seems redundant - the function call, the missing_ok
check, and the warning message already make it clear.

2/
+ * Note that the relation might not be locked, so it can be dropped
concurrently.
+ * This can happen when doing a whole database vacuum or analyze in
+ * get_all_vacuum_rels().  We issue a WARNING log message and return false in
+ * this case.

Instead of saying "relation might not be locked", can we be more
explicit about the exact cases? For example: For database-wide
vacuums, this function is called without holding a relation lock, so
the table can be dropped concurrently. In that case, issue a WARNING
and return false.

I couldn't think of a good way to reproduce this other than creating a
large number of tables to make the pg_class scan costlier. For
predictable testing I might agree to adding an injection point.
However, I have some comments:

3/
+# Make sure that current user is not the owner of current database.
+step setrole1
+{
+    SET ROLE regress_vacuum;
+}

Is this necessary?

4/
+step analyze1
+{
+    ANALYZE;
+}

I think just testing VACUUM or VACUUM ANALYZE in a single test keeps
things smaller, since the underlying code is the same for both.

5/
+++ b/src/test/modules/injection_points/specs/vacuum_concurrent_drop.spec

Why do you need isolation tests with an injection point? Why not a TAP
test under src/test/modules/test_misc? A TAP test with just VACUUM
(not ANALYZE) would keep the test simpler with fewer lines of code.

6/
+ INJECTION_POINT("vacuum-constructing-vacuumable-rels", NULL);

Nit: How about a shorter name like vacuum-get-rels or vacuum-build-rels?

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com

On Mon, Jun 15, 2026 at 02:29:57PM +0800, cca5507 wrote:

Whether a relation that disappears immediately after being added to
the list should be processed or skipped does not seem particularly
important in practice. However, taking a lock at list construction
time may make the subsequent processing simpler. I wonder whether that
would be a reasonable direction for VACUUM as well.

I don't think it's a good idea because there might be a lot of tables need
to be locked. And a concurrent drop can always happen after the list
construction because we process each table in a separate transaction.
So I think there is no need to prevent concurrent drops at list construction
time. I also prepare to write a patch to remove the lock in get_tables_to_repack().

Yeah, I doubt that forcing a lock an extra lock at an early stage is a
good thing for a manual VACUUM.

Anyway, I am wondering if we should aim for simpler. Do we really
need the extra ACL check when building a list of relations to consider
for a manual VACUUM in the pg_class scan? We are going to re-check
the permissions once we vacuum each relation in its own transaction,
*after* taking a lock on them, making the ACL check safe. That's the
vacuum_open_relation()->vacuum_is_permitted_* flow.

On a database with many relations where there is no MAINTAIN privilege
for the role running the manual VACUUM, it means more transaction
overhead because more transactions would need to be created for each
relation whose ACLs need to be rechecked, because we don't filter the
relations beforehand with the initial pg_class scan, but that would
protect from the concurrent drops entirely by limitting the check to
be in *one* path: the one analyzing or vacuuming a single relation.

I am not saying that any of this should be backpatched and that we
should treat this as a bug, a concurrent DROP reflecting on a
database-wide VACUUM is annoying, but that's not really a critical
thing to deal with. Improving that on HEAD sounds fine to me (not
v19).
--
Michael

Hi,

On Tue, Jun 23, 2026 at 4:14 PM Michael Paquier <michael@paquier.xyz> wrote:

Anyway, I am wondering if we should aim for simpler. Do we really
need the extra ACL check when building a list of relations to consider
for a manual VACUUM in the pg_class scan? We are going to re-check
the permissions once we vacuum each relation in its own transaction,
*after* taking a lock on them, making the ACL check safe. That's the
vacuum_open_relation()->vacuum_is_permitted_* flow.

On a database with many relations where there is no MAINTAIN privilege
for the role running the manual VACUUM, it means more transaction
overhead because more transactions would need to be created for each
relation whose ACLs need to be rechecked, because we don't filter the
relations beforehand with the initial pg_class scan, but that would
protect from the concurrent drops entirely by limitting the check to
be in *one* path: the one analyzing or vacuuming a single relation.

Spot on. My thinking was along similar lines. I'm concerned that in
the worst case - where the role running a database-wide vacuum has no
MAINTAIN privilege at all, or has it on only a small subset of tables
- this approach will unnecessarily do a bunch of memory allocations,
start a transaction, get a snapshot, commit the transaction, and
acquire/release locks for each relation. So, IMHO, -1 for this
approach. Instead, I would prefer using the pg_class_aclcheck_ext
check and just emitting a WARNING (like the other skipping messages)
when the relation is concurrently dropped.

That said, I'm open to hearing from others.

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com

On Tue, Jun 23, 2026 at 04:33:11PM -0700, Bharath Rupireddy wrote:

On Tue, Jun 23, 2026 at 4:14 PM Michael Paquier <michael@paquier.xyz> wrote:

Anyway, I am wondering if we should aim for simpler. Do we really
need the extra ACL check when building a list of relations to consider
for a manual VACUUM in the pg_class scan? We are going to re-check
the permissions once we vacuum each relation in its own transaction,
*after* taking a lock on them, making the ACL check safe. That's the
vacuum_open_relation()->vacuum_is_permitted_* flow.

On a database with many relations where there is no MAINTAIN privilege
for the role running the manual VACUUM, it means more transaction
overhead because more transactions would need to be created for each
relation whose ACLs need to be rechecked, because we don't filter the
relations beforehand with the initial pg_class scan, but that would
protect from the concurrent drops entirely by limitting the check to
be in *one* path: the one analyzing or vacuuming a single relation.

Spot on. My thinking was along similar lines. I'm concerned that in
the worst case - where the role running a database-wide vacuum has no
MAINTAIN privilege at all, or has it on only a small subset of tables
- this approach will unnecessarily do a bunch of memory allocations,
start a transaction, get a snapshot, commit the transaction, and
acquire/release locks for each relation. So, IMHO, -1 for this
approach. Instead, I would prefer using the pg_class_aclcheck_ext
check and just emitting a WARNING (like the other skipping messages)
when the relation is concurrently dropped.

That said, I'm open to hearing from others.

I tend to think that you are worrying too much. If a user has a small
subset of tables, they could just run a VACUUM with a list of tables
instead. That's a tradeoff of code simplicity vs cost, and I'm
finding the simplicity argument quite tempting here.

I'd sure welcome Nathan and Jeff opinions (added now in CC) regarding
this line of thoughts; they worked on MAINTAIN, even if the early
relation ACL check when building a list of relations for a
database-wide manual VACUUM predates that.
--
Michael