Re: Add SKIP LOCKED to VACUUM and ANALYZE

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: "Bossart, Nathan" <bossartn(at)amazon(dot)com>
Cc: "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, Andres Freund <andres(at)anarazel(dot)de>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>, Lyes Ameddah <lyes(dot)amd(at)gmail(dot)com>
Subject: Re: Add SKIP LOCKED to VACUUM and ANALYZE
Date: 2018-09-27 07:51:52
Message-ID: 20180927075152.GT1659@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
vacuum-open-refactor.patch text/x-diff 8.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Kapila 2018-09-27 08:00:13 Re: Problem while setting the fpw with SIGHUP
Previous Message Amit Langote 2018-09-27 07:14:35 Re: Postgres 11 release notes