On Tue, Aug 21, 2018 at 04:01:50PM +0000, Bossart, Nathan wrote: > I think my biggest concern with this approach is that we'd be > introducing inconsistent behavior whenever there are concurrent > changes. If a user never had permissions to VACUUM the partitioned > table, the partitions are skipped outright. However, if the user > loses permissions to VACUUM the partitioned table between > expand_vacuum_rel() and vacuum_rel(), we'll still attempt to VACUUM > each individual partition. > > I'll admit I don't have a great alternative proposal that doesn't > involve adding deadlock risk or complexity, but it still seems worth > mulling over.
That counts only for a manual vacuum/analyze listing directly the relation in question. If running a system-wide VACUUM then all the relations are still processed. This is a rather edge case in my opinion but.. I don't mind mulling over it (as you say). So please let me think over it for a couple of days. I don't see a smart solution which does not create risks of lock upgrades and deadlocks now, there may be one able to preserve the existing behavior. >> I have split the patch into two parts: >> - 0001 includes new tests which generate WARNING messages for VACUUM, >> ANALYZE and VACUUM (ANALYZE). That's useful separately. > > 0001 looks good to me. Thanks, I have pushed this one. >> - 0002 is the original patch discussed here. > > I'd suggest even splitting 0002 into two patches: one for refactoring > the existing permissions checks into vacuum_is_relation_owner() and > another for the new checks. Hmmm. The second patch changes also some comment blocks when calling vacuum_is_relation_owner(), so we finish by changing the same code areas, resulting in more code churn for no real gain. > +# The role doesn't have privileges to vacuum the table, so VACUUM should > +# immediately skip the table without waiting for a lock. > > Can we add tests for concurrent changes that cause the relation to be > skipped in vacuum_rel() and analyze_rel() instead of > expand_vacuum_rel()? Doing that deterministically with concurrent tests look difficult to me as doing ALTER TABLE OWNER TO to a relation in a first session causes a second session running VACUUM to block in expand_vacuum_rel(), be it with a plain table or a partitioned table (doing the ALTER TABLE on a leaf will block scanning the parent as well). -- Michael
signature.asc
Description: PGP signature
