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
