Обсуждение: Add SKIP LOCKED to VACUUM and ANALYZE

Hello,

Previous thread: https://postgr.es/m/4BC0F3CD-F4B5-4F23-AADB-80607F9E4B4E%40amazon.com

This is a new thread for tracking the work to add SKIP LOCKED to
VACUUM and ANALYZE.  I've attached a rebased set of patches.

Nathan

On Wed, Jun 13, 2018 at 08:29:12PM +0000, Bossart, Nathan wrote:
> Previous thread: https://postgr.es/m/4BC0F3CD-F4B5-4F23-AADB-80607F9E4B4E%40amazon.com
>
> This is a new thread for tracking the work to add SKIP LOCKED to
> VACUUM and ANALYZE.  I've attached a rebased set of patches.

I am beginning to look at this stuff, and committed patch 4 and 5 on the
way as they make sense independently.  I am still reviewing the rest,
which applies with some fuzz, and the tests proposed still pass.
--
Michael

On Thu, Jul 12, 2018 at 02:37:28PM +0900, Michael Paquier wrote:
> On Wed, Jun 13, 2018 at 08:29:12PM +0000, Bossart, Nathan wrote:
>> Previous thread: https://postgr.es/m/4BC0F3CD-F4B5-4F23-AADB-80607F9E4B4E%40amazon.com
>>
>> This is a new thread for tracking the work to add SKIP LOCKED to
>> VACUUM and ANALYZE.  I've attached a rebased set of patches.
>
> I am beginning to look at this stuff, and committed patch 4 and 5 on the
> way as they make sense independently.  I am still reviewing the rest,
> which applies with some fuzz, and the tests proposed still pass.

I have been looking at the rest of the patch set, and I find that the
way SKIP_LOCKED is handled is a bit inconsistent.  First, if the manual
VACUUM does not specify a list of relations, which can never happen for
autovacuum, then we get to use get_all_vacuum_rels to decide the list of
relations to look at, and then it is up to vacuum_rel() to decide if a
relation can be skipped or not.  If a list of relation is given by the
caller, then it is up to expand_vacuum_rel() to do the call.

In expand_vacuum_rel(), an OID could be defined for a relation, which is
something used by autovacuum.  If no OID is defined, which happens for a
manual VACUUM, then we use directly RangeVarGetRelidExtended() at this
stage and see if the relation is available.  If the relation listed in
the manual VACUUM is a partitioned table, then its full set of
partition OIDs (including down layers), are included in the list of
relations to include.  And we definitely want to hold, then release once
AccessShareLock so as the partition tree lookup can happen.  This uses
as well find_all_inheritors() with NoLock so as we rely on vacuum_rel()
to skip the relation or not.

The first thing which is striking me is that we may actually *not* want
to check for lock skipping within expand_vacuum_rel() as that's mainly a
function aimed at building the relations which are going to be vacuumed,
and that all the lock skipping operations should happen at the beginning
of vacuum_rel().  This way, even if the parent of a partition tree is
listed but cannot have its RangeVar opened immediately, then we have a
chance to have some of its partitions to be vacuumed after listing them.
This point is debatable as there are pros and cons.  I would be of the
opinion to not change expand_vacuum_rel()'s mission to build the list of
relations to VACUUM, and actually complain about a lock not available
when the relation is ready to be processed individually.  It is also
perfectly possible to skip locks for partitions by listing them
individually in the VACUUM command used. I am pretty sure that Andres
would complain at the sight of this paragraph as this is backwards with
the reason behind the refactoring of RangeVarGetRelidExtended but I like
the new API better :p

For this part, it seems to me that we can do better than what is in
HEAD:
- Call RangeVarGetRelidExtended without lock.
- Check for has_subclass(), which should be extended so as it does not
complain because of a missing relation so as vacuum.c does the error
handling.
- Call RangeVarGetRelidExtended a second time if a lookup with
find_all_inheritors is necessary.  If the relation goes missing
in-between then we just get an error as the current behavior.

I am also questioning the fact of complicating vac_open_indexes() by
skipping a full relation vacuum if one of its indexes cannot be opened,
which would be the case for an ALTER INDEX for example.  It seems to me
that skipping locks should just happen for the parent relation, so I
would not bother much about this case, and just document the behavior.
If somebody argues for this option, we could always have a SKIP_INDEXES
or such.

"SKIP LOCKED" should be named "SKIP_LOCKED", with an underscore for
consistency with the other options.

-void
-cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose)
+bool
+cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose,
-bool skip_locked)
 {
Some refactoring of CLUSTER on the way here?  It would be nice to avoid
having three boolean arguments here, and opens the door for an extended
CLUSTER grammar with parenthesis as well as a SKIP_LOCKED option for
it.  And this could be refactored first.  VACUUM FULL being a variant of
CLUSTER, we could just reuse the same options...  Now using separate
option names could be cleaner.

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).

I think that it would be most interesting to get the refactoring around
get_elevel_for_skip_locked and cluster_rel done first.  The improvement
for RangeVarGetRelidExtended with relations not having subclasses could
be worth done separately as well.
--
Michael

On Tue, Jul 17, 2018 at 2:21 AM, Michael Paquier <michael@paquier.xyz> wrote:
> For this part, it seems to me that we can do better than what is in
> HEAD:
> - Call RangeVarGetRelidExtended without lock.

I haven't been following this work closely, but I just want to point
out that the reason why RangeVarGetRelidExtended exists is because:

(1) we cannot lock a relation without looking up its OID, and should
not lock it without doing permissions checks first, and at least as
currently designed, those functions expect an OID, but

(2) we cannot look up a relation first and only lock it afterwards,
because DDL might happen in the middle and leave us locking the wrong
relation

When RangeVarGetRelidExtended is called with an argument other than
NoLock, it effectively makes locking, permissions checking, and the
name lookup happen simultaneously, so that it is neither possible to
lock a relation for which you don't have permissions, nor to change
the identity of the relation after the name lookup has been done and
before the lock is acquired.  On the other hand, when you call it with
NoLock, you don't get those nice behaviors.

So I'm inclined to think that any place in the source code that calls
RangeVarGetRelidExtended with NoLock is a bug, and we should be trying
to get rid of the cases we still have, not adding more.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi Michael,

Thanks for taking a look.

On 7/17/18, 1:22 AM, "Michael Paquier" <michael@paquier.xyz> wrote:
> The first thing which is striking me is that we may actually *not* want
> to check for lock skipping within expand_vacuum_rel() as that's mainly a
> function aimed at building the relations which are going to be vacuumed,
> and that all the lock skipping operations should happen at the beginning
> of vacuum_rel().  This way, even if the parent of a partition tree is
> listed but cannot have its RangeVar opened immediately, then we have a
> chance to have some of its partitions to be vacuumed after listing them.
> This point is debatable as there are pros and cons.  I would be of the
> opinion to not change expand_vacuum_rel()'s mission to build the list of
> relations to VACUUM, and actually complain about a lock not available
> when the relation is ready to be processed individually.  It is also
> perfectly possible to skip locks for partitions by listing them
> individually in the VACUUM command used. I am pretty sure that Andres
> would complain at the sight of this paragraph as this is backwards with
> the reason behind the refactoring of RangeVarGetRelidExtended but I like
> the new API better :p 

I see your point.  The only time that expand_vacuum_rel() will block
is due to a conflicting AccessExclusiveLock on the relation, and the
reason we take a lock on the relation temporarily in
expand_vacuum_rel() is documented as follows:

    /*
     * We transiently take AccessShareLock to protect the syscache lookup
     * below, as well as find_all_inheritors's expectation that the caller
     * holds some lock on the starting relation.
     */

> For this part, it seems to me that we can do better than what is in
> HEAD:
> - Call RangeVarGetRelidExtended without lock.
> - Check for has_subclass(), which should be extended so as it does not
> complain because of a missing relation so as vacuum.c does the error
> handling.
> - Call RangeVarGetRelidExtended a second time if a lookup with
> find_all_inheritors is necessary.  If the relation goes missing
> in-between then we just get an error as the current behavior.

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.

> I am also questioning the fact of complicating vac_open_indexes() by
> skipping a full relation vacuum if one of its indexes cannot be opened,
> which would be the case for an ALTER INDEX for example.  It seems to me
> that skipping locks should just happen for the parent relation, so I
> would not bother much about this case, and just document the behavior.
> If somebody argues for this option, we could always have a SKIP_INDEXES
> or such.

As long as we document this behavior, that seems reasonable to me.

> "SKIP LOCKED" should be named "SKIP_LOCKED", with an underscore for
> consistency with the other options.

I chose SKIP LOCKED with a space because both words are already
reserved and SELECT uses it this way.  I'm fine with adding an
underscore for consistency with DISABLE_PAGE_SKIPPING, though.

> -void
> -cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose)
> +bool
> +cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose,
> -bool skip_locked)
>  {
> Some refactoring of CLUSTER on the way here?  It would be nice to avoid
> having three boolean arguments here, and opens the door for an extended
> CLUSTER grammar with parenthesis as well as a SKIP_LOCKED option for
> it.  And this could be refactored first.  VACUUM FULL being a variant of
> CLUSTER, we could just reuse the same options...  Now using separate
> option names could be cleaner.

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.

Nathan

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

On Thu, Jul 19, 2018 at 10:54:23AM +0900, Michael Paquier wrote:
> 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.

The refactoring for CLUSTER is pretty obvious, and makes the API a bit
cleaner, so attached is a proposal of patch to do so.  Thoughts?
--
Michael

On 7/22/18, 10:12 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> The refactoring for CLUSTER is pretty obvious, and makes the API a bit
> cleaner, so attached is a proposal of patch to do so.  Thoughts?

Sorry for the delay on these patches!  This is nearly identical to
what I started writing last night, so it looks good to me.

+typedef enum ClusterOption
+{
+    CLUOPT_RECHECK,                /* recheck relation state */
+    CLUOPT_VERBOSE                /* print progress info */
+}            ClusterOption;

It looks like the last line here has a bit of extra whitespace
compared to the other code in parsenodes.h.

Nathan

On Mon, Jul 23, 2018 at 02:27:48PM +0000, Bossart, Nathan wrote:
> Sorry for the delay on these patches!  This is nearly identical to
> what I started writing last night, so it looks good to me.

Thanks for double-checking.  I have pushed this one to move on with the
rest of the feature.

> +typedef enum ClusterOption
> +{
> +    CLUOPT_RECHECK,                /* recheck relation state */
> +    CLUOPT_VERBOSE                /* print progress info */
> +}            ClusterOption;
>
> It looks like the last line here has a bit of extra whitespace
> compared to the other code in parsenodes.h.

That came from pgindent.  I have just updated typedefs.list to take care
of it.  There could be an argument about moving recheck out of
cluster_rel(), but that would not be nice with any module for example
calling this API, so its contract is unchanged.
--
Michael

On 7/18/18, 12:00 PM, "Bossart, Nathan" <bossartn@amazon.com> wrote:
> On 7/17/18, 1:22 AM, "Michael Paquier" <michael@paquier.xyz> wrote:
>> 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.

Here is a patch for refactoring the ereport() calls out of
vacuum_rel() and analyze_rel().  I've kept all four possible log
statements separated for ease of translation.  I considered
simplifying these statements by replacing "vacuum" and "analyze" with
"processing."  However, that seems like it could lead to ambiguity for
commands like "VACUUM (ANALYZE, SKIP_LOCKED) table1 (a);" since both
VACUUM and ANALYZE could be skipped independently.  If we add
SKIP_LOCKED to CLUSTER in the future, we will need to add two more log
statements to this function.

Nathan

On Tue, Jul 24, 2018 at 05:21:25PM +0000, Bossart, Nathan wrote:
> Here is a patch for refactoring the ereport() calls out of
> vacuum_rel() and analyze_rel().  I've kept all four possible log
> statements separated for ease of translation.  I considered
> simplifying these statements by replacing "vacuum" and "analyze" with
> "processing."  However, that seems like it could lead to ambiguity for
> commands like "VACUUM (ANALYZE, SKIP_LOCKED) table1 (a);" since both
> VACUUM and ANALYZE could be skipped independently.  If we add
> SKIP_LOCKED to CLUSTER in the future, we will need to add two more log
> statements to this function.

+typedef enum SkippedRelStmtType
+{
+   SRS_VACUUM,
+   SRS_ANALYZE
+} SkippedRelStmtType;
Hm...  I have not imagined this part but adding a new layer is sort of
ugly, and an extra one would be needed for CLUSTER as well, in which
case adding cluster-related logs into vacuum.c introduces a circular
dependency with cluster.c.  What about just skipping this refactoring
and move to the point where CLUSTER also gains its log queries directly
in cluster_rel?  VACUUM FULL is also not going to run for autovacuum, so
that's less confusion with IsAutoVacuumWorkerProcess().

Another thing is the set of issues discussed in
https://www.postgresql.org/message-id/20180724041403.GF4035%40paquier.xyz
which are actually going to touch the same code areas that we are going
to change here, for actually rather similar purposes related to lock
handling.  My gut feeling is to get the other issue fixed first, and
then rework this patch series so as we get the behavior that we want in
both the case of the previous thread and what we want here when building
a list of relations to VACUUM.  There is as well the issue where a user
does not provide a list, so that's an extra argument for fixing the
current relid fetching properly first.
--
Michael

On 7/24/18, 8:07 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> Hm...  I have not imagined this part but adding a new layer is sort of
> ugly, and an extra one would be needed for CLUSTER as well, in which
> case adding cluster-related logs into vacuum.c introduces a circular
> dependency with cluster.c.  What about just skipping this refactoring
> and move to the point where CLUSTER also gains its log queries directly
> in cluster_rel?  VACUUM FULL is also not going to run for autovacuum, so
> that's less confusion with IsAutoVacuumWorkerProcess().

Since vacuum_rel() already obtains an AccessExclusiveLock on the
relation for VACUUM FULL, we might be able to skip altering
cluster_rel().  I think we will need to alter it if we are going to
add SKIP LOCKED to CLUSTER, though.

Nathan

On Sun, Jul 29, 2018 at 10:56:24PM +0000, Bossart, Nathan wrote:
> Since vacuum_rel() already obtains an AccessExclusiveLock on the
> relation for VACUUM FULL, we might be able to skip altering
> cluster_rel().  I think we will need to alter it if we are going to
> add SKIP LOCKED to CLUSTER, though.

Nathan, could you rebase your patch set?  From what I can see the last
patch set applies with one conflict, and it would be nice for clarity to
split the routines for analyze, vacuum and cluster into separate places.
Similar to what is done with vacuum_is_relation_owner, having the same
set of logs for vacuum and analyze may be cleaner.  The set of ownership
checks should happen after the skip lock checks to be consistent between
the ownership checks done when building the relation list (list
expansion for partitions and such) as well as for vacuum_rel() and
analyze_rel().

With all the work which has been done already, I don't think that we are
that far from getting something committable.
--
Michael

On 9/3/18, 6:20 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> Nathan, could you rebase your patch set?  From what I can see the last
> patch set applies with one conflict, and it would be nice for clarity to
> split the routines for analyze, vacuum and cluster into separate places.
> Similar to what is done with vacuum_is_relation_owner, having the same
> set of logs for vacuum and analyze may be cleaner.  The set of ownership
> checks should happen after the skip lock checks to be consistent between
> the ownership checks done when building the relation list (list
> expansion for partitions and such) as well as for vacuum_rel() and
> analyze_rel().

Yes.  I've started working on this again, but the new patch set is
probably still a few days out.

> With all the work which has been done already, I don't think that we are
> that far from getting something committable.

Great!

Nathan

On Tue, Sep 04, 2018 at 03:49:09PM +0000, Bossart, Nathan wrote:
> Yes.  I've started working on this again, but the new patch set is
> probably still a few days out.

Thanks, Nathan.
--
Michael

On 9/4/18, 1:32 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
>On Tue, Sep 04, 2018 at 03:49:09PM +0000, Bossart, Nathan wrote:
>> Yes.  I've started working on this again, but the new patch set is
>> probably still a few days out.
>
> Thanks, Nathan.

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.

Nathan

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

On 9/27/18, 2:52 AM, "Michael Paquier" <michael@paquier.xyz> wrote:
> 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?

Good idea.  This patch looks good to me.

> 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.

Without the find_all_inheritors() stuff, I think we would just need to
modify the ANALYZE documentation patch to say something like

    Specifies that ANALYZE should not wait for any conflicting locks
    to be released: if a relation cannot be locked immediately without
    waiting, the relation is skipped.  Note that even with this
    option, ANALYZE can still block when opening the relation's
    indexes or when acquiring sample rows to prepare the statistics.
    Also, while ANALYZE ordinarily processes all leaf partitions of
    partitioned tables, this option will cause ANALYZE to skip all
    leaf partitions if there is a conflicting lock on the partitioned
    table.

Nathan

On Mon, Oct 01, 2018 at 03:37:01PM +0000, Bossart, Nathan wrote:
> Good idea.  This patch looks good to me.

Thanks, I have pushed this one now.

> Without the find_all_inheritors() stuff, I think we would just need to
> modify the ANALYZE documentation patch to say something like
>
>     Specifies that ANALYZE should not wait for any conflicting locks
>     to be released: if a relation cannot be locked immediately without
>     waiting, the relation is skipped.  Note that even with this
>     option, ANALYZE can still block when opening the relation's
>     indexes or when acquiring sample rows to prepare the statistics.
>     Also, while ANALYZE ordinarily processes all leaf partitions of
>     partitioned tables, this option will cause ANALYZE to skip all
>     leaf partitions if there is a conflicting lock on the partitioned
>     table.

Yes, I think that we could live with something like that.  I could give
this whole thing a shot, but this will have to wait until the commit
fest is closed as there are still many patches to classify.  If you can
send a patch that's of course always helpful..
--
Michael

On 10/1/18, 7:07 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> On Mon, Oct 01, 2018 at 03:37:01PM +0000, Bossart, Nathan wrote:
>> Without the find_all_inheritors() stuff, I think we would just need to
>> modify the ANALYZE documentation patch to say something like
>> 
>>     Specifies that ANALYZE should not wait for any conflicting locks
>>     to be released: if a relation cannot be locked immediately without
>>     waiting, the relation is skipped.  Note that even with this
>>     option, ANALYZE can still block when opening the relation's
>>     indexes or when acquiring sample rows to prepare the statistics.
>>     Also, while ANALYZE ordinarily processes all leaf partitions of
>>     partitioned tables, this option will cause ANALYZE to skip all
>>     leaf partitions if there is a conflicting lock on the partitioned
>>     table.
>
> Yes, I think that we could live with something like that.  I could give
> this whole thing a shot, but this will have to wait until the commit
> fest is closed as there are still many patches to classify.  If you can
> send a patch that's of course always helpful..

Sure, here it is.

Nathan

On Tue, Oct 02, 2018 at 02:22:42AM +0000, Bossart, Nathan wrote:
> On 10/1/18, 7:07 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
>> On Mon, Oct 01, 2018 at 03:37:01PM +0000, Bossart, Nathan wrote:
>>> Without the find_all_inheritors() stuff, I think we would just need to
>>> modify the ANALYZE documentation patch to say something like
>>>
>>>     Specifies that ANALYZE should not wait for any conflicting locks
>>>     to be released: if a relation cannot be locked immediately without
>>>     waiting, the relation is skipped.  Note that even with this
>>>     option, ANALYZE can still block when opening the relation's
>>>     indexes or when acquiring sample rows to prepare the statistics.
>>>     Also, while ANALYZE ordinarily processes all leaf partitions of
>>>     partitioned tables, this option will cause ANALYZE to skip all
>>>     leaf partitions if there is a conflicting lock on the partitioned
>>>     table.
>>
>> Yes, I think that we could live with something like that.  I could give
>> this whole thing a shot, but this will have to wait until the commit
>> fest is closed as there are still many patches to classify.  If you can
>> send a patch that's of course always helpful..
>
> Sure, here it is.

I was just playing with autovacuum and some inheritance trees, and
actually locking a child would cause an autovacuum worker to block if it
is analyzing the parent when acquiring row samples even now.  Anyway,
coming back to the patch...

My comments are mostly about the documentation bits added:
- Inheritance trees have the same problem, so it would be nice to
mention them as well.
- One workaround, which we could document (?), would be to list leaves
of a partition tree individually so as their locks are checked one by
one.

+      Specifies that <command>ANALYZE</command> should not wait for any
+      conflicting locks to be released: if a relation cannot be locked

Perhaps it would be more precise to tell when "beginning to work on a
relation" because the restrictions in row samplings?

The rest of the patch looks clean to me.
--
Michael

On 10/3/18, 12:54 AM, "Michael Paquier" <michael@paquier.xyz> wrote:
> - Inheritance trees have the same problem, so it would be nice to
> mention them as well.

Done.

> - One workaround, which we could document (?), would be to list leaves
> of a partition tree individually so as their locks are checked one by
> one.

Done.

> +      Specifies that <command>ANALYZE</command> should not wait for any
> +      conflicting locks to be released: if a relation cannot be locked
>
> Perhaps it would be more precise to tell when "beginning to work on a
> relation" because the restrictions in row samplings?

Done.

Here is what I have so far for the docs:

        Specifies that ANALYZE should not wait for any conflicting
        locks to be released before beginning work on a relation: if a
        relation cannot be locked immediately without waiting, the
        relation is skipped.  Note that even with this option, ANALYZE
        may still block when opening the relation's indexes or when
        acquiring sample rows from leaf partitions, table inheritance
        children, and some types of foreign tables.  Also, while
        ANALYZE ordinarily processes all leaf partitions of specified
        partitioned tables, this option will cause ANALYZE to skip all
        leaf partitions if there is a conflicting lock on the
        partitioned table.  Therefore, it is recommended to list each
        leaf partition individually in the ANALYZE command when using
        the SKIP_LOCKED option.

        Specifies that VACUUM should not wait for any conflicting
        locks to be released before beginning work on a relation: if a
        relation cannot be locked immediately without waiting, the
        relation is skipped.  Note that even with this option, VACUUM
        may still block when opening the relation's indexes.  Also,
        while VACUUM ordinarily processes all leaf partitions of
        specified partitioned tables, this option will cause VACUUM to
        skip all leaf partitions if there is a conflicting lock on the
        partitioned table.  Therefore, it is recommended to list each
        leaf partition individually in the VACUUM command when using
        the SKIP_LOCKED option.

Nathan

On Wed, Oct 03, 2018 at 04:20:42PM +0000, Bossart, Nathan wrote:
> Here is what I have so far for the docs:
>
> [snip]

Thanks, I have committed the patch, after minor tweaks to the docs.
Mainly, in the VACUUM page, I have added a mention that VACUUM ANALYZE
can block for row sampling.  I have also removed for now the set of
recommendations the patch added, as we still gather statistics on the
partitioned tables from the row samples gathered from the partitions, so
recommending to list only the leaves is not completely correct either
all the time.

We could perhaps add something about such recommendations.  A separate
section covering more general things may be interesting.
--
Michael

On 10/3/18, 7:10 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> Thanks, I have committed the patch, after minor tweaks to the docs.

Thanks!

> Mainly, in the VACUUM page, I have added a mention that VACUUM ANALYZE
> can block for row sampling.  I have also removed for now the set of
> recommendations the patch added, as we still gather statistics on the
> partitioned tables from the row samples gathered from the partitions, so
> recommending to list only the leaves is not completely correct either
> all the time.

Good catch.  The committed version of the documentation looks good to
me.

> We could perhaps add something about such recommendations.  A separate
> section covering more general things may be interesting.

Agreed, this could be a good addition to the "Notes" sections.

Nathan