Hi, On Fri, Jun 26, 2026 at 2:36 AM cca5507 <[email protected]> wrote: > > Hi, > > > > 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 > > > > FWIW, I agree with this direction. > > This direction is also OK for me. We might want to fix get_tables_to_repack() > together?
We want to get the vacuum behavior consistent across all three modes - database-wide vacuum, vacuum <tables-list>, and autovacuum - as far as the permissions check is concerned. That is, do the permissions check after acquiring the table-level lock. This also solves the ERRORs reported due to concurrent table drops during the permissions check, which happen because pg_class_aclcheck is used instead of pg_class_aclcheck_ext with is_missing. I had a quick look at the repack code and it seems like it can also run into the same problem because repack_is_permitted_for_relation uses the same pg_class_aclcheck while building the tables list without holding locks, and later it rechecks permissions after the table locks in cluster_rel_recheck anyway. A simple fix there would be to just use pg_class_aclcheck_ext in repack_is_permitted_for_relation. I recommend discussing this in a separate thread CC-ing the repack authors to get agreement. Do you mind sharing the vacuum-related fix where we have agreement in this thread? Thank you! -- Bharath Rupireddy Amazon Web Services: https://aws.amazon.com
