Re: Improve behavior of concurrent ANALYZE/VACUUM

On 8/21/18, 7:44 PM, "Michael Paquier" <michael(at)paquier(dot)xyz> wrote:
> 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.

Right. If we don't come up with anything, the behavior change for
this edge case is probably reasonable as long as we update the
documentation like you proposed earlier.

>>> - 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.

I see. I only made this suggestion so that we could get some of the
easy changes out of the way, but there's no need if it's just adding
unnecessary code churn.

>> +# 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).

I think this is doable by locking the table in SHARE mode. That won't
conflict with the AccessShareLock that expand_vacuum_rel() obtains,
but it will conflict with the ShareUpdateExclusiveLock or
AccessExclusiveLock that vacuum_rel() takes.

session 1> BEGIN; LOCK test IN SHARE MODE;
session 2> VACUUM test;
session 1> ALTER TABLE test OWNER TO not_session_2_user; COMMIT;

Nathan