Re: Add SKIP LOCKED to VACUUM and ANALYZE

On Wed, Jul 18, 2018 at 04:58:48PM +0000, Bossart, Nathan wrote:
> On 7/17/18, 1:22 AM, "Michael Paquier" <michael(at)paquier(dot)xyz> wrote:
> Perhaps we could extend RangeVarGetRelidExtended() to only lock if
> has_subclass() is true. However, I also understand Robert's position
> on calling RangeVarGetRelidExtended() with NoLock, and I'm not sure if
> this locking optimization is worth the complexity.

Yeah, I am having as well second-thoughts about this remark of mine.
This would introduce more complexity so it may be better to just do as
you suggested previously to skip the parent locked. Getting that
documented would be nice, in the lines of for example "If a partitioned
relation is listed in the set provided to VACUUM, then the whole tree of
relations to vacuum will be considered. When using SKIP_LOCKED, if the
parent cannot be locked when building this relation list, then none of
its partitions are vacuumed and the only the parent is skipped. If the
partitioned relation can be locked at its set of partitions listed, then
all partitions will be considered for vacuuming, and each partition
will be considered by SKIP_LOCKED individually when its VACUUM begins".

I am pretty sure you would come up with better words than those written
quickly.

> Refactoring the boolean arguments of cluster_rel() into an options
> argument seems like a good idea.
>
>> The stuff of get_elevel_for_skipped_relation could be refactored into
>> something used as well by cluster_rel as the same logic is used in three
>> places (vacuum_rel, analyze_rel and cluster_rel with try_relation_open).
>
> This seems like a good idea if we intend to add SKIP LOCKED to CLUSTER
> eventually, and it would be nice to cut down on some of the duplicated
> ereport() calls. I'll look into it.

If you can get this refactoring done, let's look into getting those two
parts committed first to simplify the next steps. No need to extend the
grammar of cluster either. The set of options for CLUSTER should be a
separate enum list on top of ClusterStmt in parsenodes.h, as VACUUM has
no recheck option. I would recommend to get cluster_rel reshaped first,
and the ereport() calls done after.
--
Michael