Hi, On Wed, Jun 24, 2026 at 11:20 PM Michael Paquier <[email protected]> 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
