Re: Handle concurrent drop when doing whole database vacuum

Hi,

On Wed, Jun 24, 2026 at 11:20 PM Michael Paquier <michael(at)paquier(dot)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