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
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index d11b559b20..da757f0974 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -120,7 +120,6 @@ analyze_rel(Oid relid, RangeVar *relation, int options,
int elevel;
AcquireSampleRowsFunc acquirefunc = NULL;
BlockNumber relpages = 0;
- bool rel_lock = true;
/* Select logging level */
if (options & VACOPT_VERBOSE)
@@ -142,58 +141,16 @@ analyze_rel(Oid relid, RangeVar *relation, int options,
* concurrent VACUUM, which doesn't matter much at the moment but might
* matter if we ever try to accumulate stats on dead tuples.) If the rel
* has been dropped since we last saw it, we don't need to process it.
+ *
+ * Make sure to generate only logs for ANALYZE in this case.
*/
- if (!(options & VACOPT_SKIP_LOCKED))
- onerel = try_relation_open(relid, ShareUpdateExclusiveLock);
- else if (ConditionalLockRelationOid(relid, ShareUpdateExclusiveLock))
- onerel = try_relation_open(relid, NoLock);
- else
- {
- onerel = NULL;
- rel_lock = false;
- }
+ onerel = vacuum_open_relation(relid, relation, params,
+ options & ~(VACOPT_VACUUM),
+ ShareUpdateExclusiveLock);
- /*
- * If we failed to open or lock the relation, emit a log message before
- * exiting.
- */
+ /* leave if relation could not be opened or locked */
if (!onerel)
- {
- /*
- * If the RangeVar is not defined, we do not have enough information
- * to provide a meaningful log statement. Chances are that
- * analyze_rel's caller has intentionally not provided this
- * information so that this logging is skipped, anyway.
- */
- if (relation == NULL)
- return;
-
- /*
- * Determine the log level. For autovacuum logs, we emit a LOG if
- * log_autovacuum_min_duration is not disabled. For manual ANALYZE,
- * we emit a WARNING to match the log statements in the permissions
- * checks.
- */
- if (!IsAutoVacuumWorkerProcess())
- elevel = WARNING;
- else if (params->log_min_duration >= 0)
- elevel = LOG;
- else
- return;
-
- if (!rel_lock)
- ereport(elevel,
- (errcode(ERRCODE_LOCK_NOT_AVAILABLE),
- errmsg("skipping analyze of \"%s\" --- lock not available",
- relation->relname)));
- else
- ereport(elevel,
- (errcode(ERRCODE_UNDEFINED_TABLE),
- errmsg("skipping analyze of \"%s\" --- relation no longer exists",
- relation->relname)));
-
return;
- }
/*
* Check if relation needs to be skipped based on ownership. This check
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index f166509734..4e3823b0f0 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -482,6 +482,112 @@ vacuum_is_relation_owner(Oid relid, Form_pg_class reltuple, int options)
}
+/*
+ * vacuum_open_relation
+ *
+ * This routine is used for attempting to open and lock a relation which
+ * is going to be vacuumed or analyzed. If the relation cannot be opened
+ * or locked, a log is emitted if possible.
+ */
+Relation
+vacuum_open_relation(Oid relid, RangeVar *relation, VacuumParams *params,
+ int options, LOCKMODE lmode)
+{
+ Relation onerel;
+ bool rel_lock = true;
+ int elevel;
+
+ Assert(params != NULL);
+ Assert((options & (VACOPT_VACUUM | VACOPT_ANALYZE)) != 0);
+
+ /*
+ * Open the relation and get the appropriate lock on it.
+ *
+ * There's a race condition here: the relation may have gone away since
+ * the last time we saw it. If so, we don't need to vacuum or analyze it.
+ *
+ * If we've been asked not to wait for the relation lock, acquire it first
+ * in non-blocking mode, before calling try_relation_open().
+ */
+ if (!(options & VACOPT_SKIP_LOCKED))
+ onerel = try_relation_open(relid, lmode);
+ else if (ConditionalLockRelationOid(relid, lmode))
+ onerel = try_relation_open(relid, NoLock);
+ else
+ {
+ onerel = NULL;
+ rel_lock = false;
+ }
+
+ /* if relation is opened, leave */
+ if (onerel)
+ return onerel;
+
+ /*
+ * Relation could not be opened, hence generate if possible a log
+ * informing on the situation.
+ *
+ * If the RangeVar is not defined, we do not have enough information to
+ * provide a meaningful log statement. Chances are that the caller has
+ * intentionally not provided this information so that this logging is
+ * skipped, anyway.
+ */
+ if (relation == NULL)
+ return NULL;
+
+ /*
+ * Determine the log level.
+ *
+ * For autovacuum logs, we emit a LOG if log_autovacuum_min_duration is
+ * not disabled. For manual VACUUM or ANALYZE, we emit a WARNING to match
+ * the log statements in the permission checks.
+ */
+ if (!IsAutoVacuumWorkerProcess())
+ elevel = WARNING;
+ else if (params->log_min_duration >= 0)
+ elevel = LOG;
+ else
+ return NULL;
+
+ if ((options & VACOPT_VACUUM) != 0)
+ {
+ if (!rel_lock)
+ ereport(elevel,
+ (errcode(ERRCODE_LOCK_NOT_AVAILABLE),
+ errmsg("skipping vacuum of \"%s\" --- lock not available",
+ relation->relname)));
+ else
+ ereport(elevel,
+ (errcode(ERRCODE_UNDEFINED_TABLE),
+ errmsg("skipping vacuum of \"%s\" --- relation no longer exists",
+ relation->relname)));
+
+ /*
+ * For VACUUM ANALYZE, both logs could show up, but just generate
+ * information for VACUUM as that would be the first one to be
+ * processed.
+ */
+ return NULL;
+ }
+
+ if ((options & VACOPT_ANALYZE) != 0)
+ {
+ if (!rel_lock)
+ ereport(elevel,
+ (errcode(ERRCODE_LOCK_NOT_AVAILABLE),
+ errmsg("skipping analyze of \"%s\" --- lock not available",
+ relation->relname)));
+ else
+ ereport(elevel,
+ (errcode(ERRCODE_UNDEFINED_TABLE),
+ errmsg("skipping analyze of \"%s\" --- relation no longer exists",
+ relation->relname)));
+ }
+
+ return NULL;
+}
+
+
/*
* Given a VacuumRelation, fill in the table OID if it wasn't specified,
* and optionally add VacuumRelations for partitions of the table.
@@ -1400,7 +1506,6 @@ vacuum_rel(Oid relid, RangeVar *relation, int options, VacuumParams *params)
Oid save_userid;
int save_sec_context;
int save_nestlevel;
- bool rel_lock = true;
Assert(params != NULL);
@@ -1455,68 +1560,12 @@ vacuum_rel(Oid relid, RangeVar *relation, int options, VacuumParams *params)
*/
lmode = (options & VACOPT_FULL) ? AccessExclusiveLock : ShareUpdateExclusiveLock;
- /*
- * Open the relation and get the appropriate lock on it.
- *
- * There's a race condition here: the rel may have gone away since the
- * last time we saw it. If so, we don't need to vacuum it.
- *
- * If we've been asked not to wait for the relation lock, acquire it first
- * in non-blocking mode, before calling try_relation_open().
- */
- if (!(options & VACOPT_SKIP_LOCKED))
- onerel = try_relation_open(relid, lmode);
- else if (ConditionalLockRelationOid(relid, lmode))
- onerel = try_relation_open(relid, NoLock);
- else
- {
- onerel = NULL;
- rel_lock = false;
- }
+ /* open the relation and get the appropriate lock on it */
+ onerel = vacuum_open_relation(relid, relation, params, options, lmode);
- /*
- * If we failed to open or lock the relation, emit a log message before
- * exiting.
- */
+ /* leave if relation could not be opened or locked */
if (!onerel)
{
- int elevel = 0;
-
- /*
- * Determine the log level.
- *
- * If the RangeVar is not defined, we do not have enough information
- * to provide a meaningful log statement. Chances are that
- * vacuum_rel's caller has intentionally not provided this information
- * so that this logging is skipped, anyway.
- *
- * Otherwise, for autovacuum logs, we emit a LOG if
- * log_autovacuum_min_duration is not disabled. For manual VACUUM, we
- * emit a WARNING to match the log statements in the permission
- * checks.
- */
- if (relation != NULL)
- {
- if (!IsAutoVacuumWorkerProcess())
- elevel = WARNING;
- else if (params->log_min_duration >= 0)
- elevel = LOG;
- }
-
- if (elevel != 0)
- {
- if (!rel_lock)
- ereport(elevel,
- (errcode(ERRCODE_LOCK_NOT_AVAILABLE),
- errmsg("skipping vacuum of \"%s\" --- lock not available",
- relation->relname)));
- else
- ereport(elevel,
- (errcode(ERRCODE_UNDEFINED_TABLE),
- errmsg("skipping vacuum of \"%s\" --- relation no longer exists",
- relation->relname)));
- }
-
PopActiveSnapshot();
CommitTransactionCommand();
return false;
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 5af96fdc8a..2f4303e40d 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -188,6 +188,8 @@ extern void vac_update_datfrozenxid(void);
extern void vacuum_delay_point(void);
extern bool vacuum_is_relation_owner(Oid relid, Form_pg_class reltuple,
int options);
+extern Relation vacuum_open_relation(Oid relid, RangeVar *relation,
+ VacuumParams *params, int options, LOCKMODE lmode);
/* in commands/vacuumlazy.c */
extern void lazy_vacuum_rel(Relation onerel, int options,
signature.asc
Description: PGP signature
