On Wed, Jul 18, 2018 at 04:58:48PM +0000, Bossart, Nathan wrote: > On 7/17/18, 1:22 AM, "Michael Paquier" <mich...@paquier.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
signature.asc
Description: PGP signature