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