On 8/20/18, 8:29 PM, "Michael Paquier" <mich...@paquier.xyz> wrote: >> In short, my vote would be to maintain the current behavior for now >> and to bring up any logging improvements separately. > > On the other hand, it would be useful for the user to know exactly what > is getting skipped. For example if VACUUM ANALYZE is used then both > operations would happen, but now the user would only know that VACUUM > has been skipped, and may miss the fact that ANALYZE was not attempted. > Let's do as you suggest at the end, aka if both VACOPT_VACUUM and > VACOPT_ANALYZE are passed down to vacuum_is_relation_owner, then only > the log for VACUUM is generated, which is consistent. Any other changes > could happen later on if necessary.
Sounds good. > If we don't want to change the current behavior, then one simple > solution would be close to what you mention, aka skip adding the > partitioned table to the list, include *all* the partitions in the list > as we cannot sanely check their ACLs at this stage, and rely on the > checks already happening in vacuum_rel() and analyze_rel(). This would > cause the original early lock attempts to not be solved for partitions, > which is why the approach taken in the patches makes the most sense. 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. > 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. > - 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. +# 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()? Nathan
