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

Hi Michael,

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.

It seems that the original permission check in get_all_vacuum_rels() is added
by you in commit a556549d7e6dce15fe216bd4130ea64239f4d83f.

--
Regards,
ChangAo Chen

On Wed, Jun 24, 2026 at 11:21:46AM +0800, cca5507 wrote:

It seems that the original permission check in get_all_vacuum_rels() is added
by you in commit a556549d7e6dce15fe216bd4130ea64239f4d83f.

Aye, I'm aware of that. :D
--
Michael

Hi ChangAo,

Thank you for incorporating the changes.

I applied and tested the v3 patch, it works all good.

Both permutations (vacuum and analyze) pass and the expected output now
asserts 's1: WARNING: skipping vacuum of "foo" and' 's1: WARNING:
skipping analyze of "foo" ' respectively which looks correct.

Regarding the backpatch, I agree this is a long standing bug, the test spec
might need to change as there is no pg_maintain, but the fix itself should
apply cleanly.

Overall, the patch looks in good shape.
Thank you for working on this issue.

You can create a commitfest entry.

Regards,
Surya Poondla

Hi,

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.

Yeah, the code can explain itself clearly, so removed.

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.

Fixed.

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?

Yes, we won't check MAINTAIN privilege on the table if current user is the
owner of current database.

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.

Yeah, the ANALYZE might also increase the test time, so removed.

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.

The src/test/perl/README says:

It's used to drive tests for backup and restore, replication, etc - anything that can't
really be expressed using pg_regress or the isolation test framework.

I can write a TAP test if we decide to use it.

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

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

I think the current one is ok, so I didn't change this.

--
Regards,
ChangAo Chen

On Thu, Jun 25, 2026 at 01:53:39PM +0800, cca5507 wrote:

I think the current one is ok, so I didn't change this.

+ * Note: this function is called without holding a relation lock for database-wide
+ * VACUUM or ANALYZE, so the relation can be dropped concurrently. In that
+ * case, issue a WARNING and return false.

FWIW, even after sleeping on it, I don't think that this patch is
going in the right direction. I am pretty sure that we should just
lift the ACL check in get_all_vacuum_rels() and always require
vacuum_is_permitted_for_relation() to have the relation locked when
called. This way we would rely on one single code path for the ACL
check, even if it means holding a reference of a relation OID for
something to be processsed later. So I would revisit the choice made
in a556549d7e6d. The isolation test vacuum-conflict also seems OK
with this shortcut, after a quick test, which was the kind of patterns
this commit is after.
--
Michael

Hi,

On Wed, Jun 24, 2026 at 11:20 PM Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Jun 25, 2026 at 01:53:39PM +0800, cca5507 wrote:

I think the current one is ok, so I didn't change this.

+ * Note: this function is called without holding a relation lock for database-wide
+ * VACUUM or ANALYZE, so the relation can be dropped concurrently. In that
+ * case, issue a WARNING and return false.

FWIW, even after sleeping on it, I don't think that this patch is
going in the right direction. I am pretty sure that we should just
lift the ACL check in get_all_vacuum_rels() and always require
vacuum_is_permitted_for_relation() to have the relation locked when
called. This way we would rely on one single code path for the ACL
check, even if it means holding a reference of a relation OID for
something to be processsed later. So I would revisit the choice made
in a556549d7e6d.

After thinking about it again, I'd take back my concern about the
"unnecessary work" if the ACL check is removed from
get_all_vacuum_rels() and left to vacuum_rel()'s
vacuum_is_permitted_for_relation() after the table lock. My concern
was around a rare use-case, and the other vacuum invocations (vacuum
<table-list> and autovacuum) didn't have this problem anyway.

I now fully agree with your point on making it consistent across all
vacuum invocations (database-wide, with table list, and autovacuum).
In this case, the 0001 patch could just remove the
vacuum_is_permitted_for_relation call from get_all_vacuum_rels, with a
comment noting that permission checks are done in vacuum_rel.

The isolation test vacuum-conflict also seems OK
with this shortcut, after a quick test, which was the kind of patterns
this commit is after.

One comment on the isolation test - why an isolation test? Why not a
TAP test under src/test/modules/test_misc? IMO, TAP test is easier to
maintain (no expected output file) and fewer LoC. We may not need a
new test file either - the closest place to add it would be
006_signal_autovacuum.pl or add a 100_bugs.pl or such.

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

Hi Michael, Bharath, ChangAo,

I tested v3 locally previously, but after thinking through Michael's
argument

FWIW, even after sleeping on it, I don't think that this patch is

going in the right direction. I am pretty sure that we should just
lift the ACL check in get_all_vacuum_rels() and always require
vacuum_is_permitted_for_relation() to have the relation locked when
called. This way we would rely on one single code path for the ACL
check, even if it means holding a reference of a relation OID for
something to be processsed later. So I would revisit the choice made
in a556549d7e6d.

I agree the architectural direction is better.
Removing the early ACL check from get_all_vacuum_rels() means the syscache
lookup that was racing with concurrent DROP never happens at list
construction
at all. The race altogether vanishes and we don't need to handle the
is_missing.
Also it brings the database-wide VACUUM path in line with VACUUM
<table-list> and autovacuum.

On the partial MAINTAIN case, the per-relation transaction cost is real,
and the rejected vacuum_rel calls would still take ShareUpdateExclusiveLock
briefly, which
could contend with autovacuum on tables the user can't vacuum anyway.
The architectural simplification is durable. Seems like the right tradeoff
as this partial MAINTAIN configuration is not widely used.

On the test front: once the race code path is gone, the injection point
with the spec file isn't strictly needed either.
A simpler test that shows "VACUUM with a concurrent drop successfully
completes without error" should suffice.
Bharath's TAP test suggestion under src/test/modules/test_misc seems like a
good fit now.

Regards,
Surya Poondla

Hi,

On Thu, Jun 25, 2026 at 10:41 AM surya poondla <suryapoondla4@gmail.com> wrote:

On the test front: once the race code path is gone, the injection point with the spec file isn't strictly needed either.
A simpler test that shows "VACUUM with a concurrent drop successfully completes without error" should suffice.
Bharath's TAP test suggestion under src/test/modules/test_misc seems like a good fit now.

Thinking about this further, the test is NOT needed at all with this
approach because vacuum_open_relation() already protects against
concurrent drops. So, -1 for adding a new test.

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

On Thu, Jun 25, 2026 at 11:04:55AM -0700, Bharath Rupireddy wrote:

On Thu, Jun 25, 2026 at 10:41 AM surya poondla <suryapoondla4@gmail.com> wrote:

On the test front: once the race code path is gone, the injection point with the spec file isn't strictly needed either.
A simpler test that shows "VACUUM with a concurrent drop successfully completes without error" should suffice.
Bharath's TAP test suggestion under src/test/modules/test_misc seems like a good fit now.

Thinking about this further, the test is NOT needed at all with this
approach because vacuum_open_relation() already protects against
concurrent drops. So, -1 for adding a new test.

Well, as you mentioned upthread the only test infrastructure that we
could use here is TAP, as database-wide VACUUMs could get costly when
running other tests in parallel and we need stability. But I cannot
get excited about the cost this would have versus the coverage we get.
Cycles are better spent elsewhere, even if injection points could be
leveraged here.
--
Michael