Re: Improve behavior of concurrent ANALYZE/VACUUM

On 8/20/18, 8:29 PM, "Michael Paquier" <michael@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