On Wed, Sep 05, 2018 at 03:24:21PM +0000, Bossart, Nathan wrote:
> And here it is. Here is a summary of the notable changes:
>
> 1) Patches v8-0003 and v8-0008 have been discarded. These patches
> added SKIP_LOCKED behavior when opening a relation's indexes.
> Instead, I've documented that VACUUM and ANALYZE may still block
> on indexes in v9-0007.
> 2) Patches v8-0004 and v8-0005 have been discarded, as they have
> already been committed.
> 3) Patch v8-0011 has been discarded. As previously noted, VACUUM
> (SKIP_LOCKED, FULL) is already handled in vacuum_rel(), so no
> changed are required to cluster_rel(). However, we will need
> something similar to v8-0011 if we ever add SKIP_LOCKED to
> CLUSTER.
> 4) The option has been renamed to SKIP_LOCKED (with the underscore)
> for consistency with the DISABLE_PAGE_SKIPPING option.
> 5) In the documentation, I've listed the caveats for SKIP_LOCKED and
> partitioned tables. I tried to make all the new documentation as
> concise as possible.
Thanks for the new patches. So I have begun looking at the full set,
beginning by 0001 which introduces a new common routine to get the
elevel associated to a skipped relation. I have been chewing on this
one, and I think that the patch could do more in terms of refactoring by
introducing one single routine able to open a relation which is going to
be vacuumed or analyzed. This removes quite a lot of duplication
between analyze_rel() and vacuum_rel() when it comes to using
try_relation_open(). The result is attached, and that makes the code
closer to what the recently-added vacuum_is_relation_owner does.
Nathan, what do you think?
Please note that I am on the edge of discarding the stuff in
find_all_inheritors and such, as well as not worrying about the case of
analyze which could block for some index columns, but I have not spent
much time studying this part yet. Still the potential issues around
complicating this code, particularly when things come to having a
partial analyze or vacuum done rather scares me.
--
Michael