Handle concurrently-dropped relations in database-wide VACUUM.

When VACUUM or ANALYZE is run without a table list, we scan
pg_class to build the list of relations to process, and we
check the privileges on each relation we find.  Since we don't
take any locks on the relations at this point, it's possible
for one to be concurrently dropped, in which case the privilege
check fails with an ERROR such as the following:

	ERROR:  relation with OID 16388 does not exist

This unnecessarily aborts the entire command.  To fix, use
pg_class_aclcheck_ext() for the privilege check so that we can
detect concurrently-dropped relations and silently skip them.
There's no need to emit a WARNING for such relations because
they weren't explicitly named, and a drop at this point is no
different than one that happened before our pg_class scan
began.  Note that concurrent drops that occur later on are
already handled gracefully by vacuum_open_relation().

The new missing_ok behavior is limited to get_all_vacuum_rels().
All other callers of vacuum_is_permitted_for_relation() should
hold a lock on the relation that prevents it from being
concurrently dropped, so this commit also adds an assertion to
that effect.

Oversight in commit a556549d7e.

This is a bug fix and could be back-patched, but given the
presumed rarity of the race condition and the lack of field
reports, I'm not going to bother.

Reported-by: ChangAo Chen <cca5507@qq.com>
Author: ChangAo Chen <cca5507@qq.com>
Co-authored-by: Nathan Bossart <nathandbossart@gmail.com>
Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Reviewed-by: Surya Poondla <suryapoondla4@gmail.com>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/tencent_F9D483523BB0D082C2EFDA80142F192DBC07%40qq.com

diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index f66e80b757cbf3f3b46ec3f5e517a1801504d978..c28b9dae9833cabfdf3752ce5bb46a42c3c1c885 100644 (file)
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -157,7 +157,8 @@ analyze_rel(Oid relid, RangeVar *relation,
         */
        if (!vacuum_is_permitted_for_relation(RelationGetRelid(onerel),
                                                                                  onerel->rd_rel,
         */
        if (!vacuum_is_permitted_for_relation(RelationGetRelid(onerel),
                                                                                  onerel->rd_rel,

-                                                                                 params->options & ~VACOPT_VACUUM))
+                                                                                 params->options & ~VACOPT_VACUUM,
+                                                                                 false))

        {
                relation_close(onerel, ShareUpdateExclusiveLock);
                return;
        {
                relation_close(onerel, ShareUpdateExclusiveLock);
                return;

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 38539a6fd3d4ee109ef004b9a1fcbddc9f485f3e..31f9824899cfbc150d3e4eb329155c11dcd4d252 100644 (file)
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -715,14 +715,21 @@ vacuum(List *relations, const VacuumParams *params, BufferAccessStrategy bstrate
  * If not, issue a WARNING log message and return false to let the caller
  * decide what to do with this relation.  This routine is used to decide if a
  * relation can be processed for VACUUM or ANALYZE.
  * If not, issue a WARNING log message and return false to let the caller
  * decide what to do with this relation.  This routine is used to decide if a
  * relation can be processed for VACUUM or ANALYZE.

+ *
+ * If missing_ok is true, we silently return false if the relation is
+ * concurrently dropped.  Callers without a lock on the relation must specify
+ * missing_ok; all others must hold at least AccessShareLock.

  */
 bool
 vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple,
  */
 bool
 vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple,

-                                                                uint32 options)
+                                                                uint32 options, bool missing_ok)

 {
        char       *relname;
 {
        char       *relname;

+       bool            is_missing = false;

 
        Assert((options & (VACOPT_VACUUM | VACOPT_ANALYZE)) != 0);
 
        Assert((options & (VACOPT_VACUUM | VACOPT_ANALYZE)) != 0);

+       Assert(missing_ok ||
+                  CheckRelationOidLockedByMe(relid, AccessShareLock, true));

 
        /*----------
         * A role has privileges to vacuum or analyze the relation if any of the
 
        /*----------
         * A role has privileges to vacuum or analyze the relation if any of the


@@ -733,9 +740,20 @@ vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple,
         */
        if ((object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) &&
                 !reltuple->relisshared) ||
         */
        if ((object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) &&
                 !reltuple->relisshared) ||

-               pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) == ACLCHECK_OK)
+               pg_class_aclcheck_ext(relid, GetUserId(), ACL_MAINTAIN,
+                                                         missing_ok ? &is_missing : NULL) == ACLCHECK_OK)

                return true;
 
                return true;
 

+       /*
+        * If the relation was concurrently dropped, nothing to do.  Note that
+        * this is only reachable when the caller specified missing_ok.
+        */
+       if (is_missing)
+       {
+               Assert(missing_ok);
+               return false;
+       }
+

        relname = NameStr(reltuple->relname);
 
        if ((options & VACOPT_VACUUM) != 0)
        relname = NameStr(reltuple->relname);
 
        if ((options & VACOPT_VACUUM) != 0)


@@ -956,7 +974,7 @@ expand_vacuum_rel(VacuumRelation *vrel, MemoryContext vac_context,
                 * Make a returnable VacuumRelation for this rel if the user has the
                 * required privileges.
                 */
                 * Make a returnable VacuumRelation for this rel if the user has the
                 * required privileges.
                 */

-               if (vacuum_is_permitted_for_relation(relid, classForm, options))
+               if (vacuum_is_permitted_for_relation(relid, classForm, options, false))

                {
                        oldcontext = MemoryContextSwitchTo(vac_context);
                        vacrels = lappend(vacrels, makeVacuumRelation(vrel->relation,
                {
                        oldcontext = MemoryContextSwitchTo(vac_context);
                        vacrels = lappend(vacrels, makeVacuumRelation(vrel->relation,


@@ -1069,7 +1087,7 @@ get_all_vacuum_rels(MemoryContext vac_context, int options)
                        continue;
 
                /* check permissions of relation */
                        continue;
 
                /* check permissions of relation */

-               if (!vacuum_is_permitted_for_relation(relid, classForm, options))
+               if (!vacuum_is_permitted_for_relation(relid, classForm, options, true))

                        continue;
 
                /*
                        continue;
 
                /*


@@ -2115,7 +2133,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams params,
         */
        if (!vacuum_is_permitted_for_relation(priv_relid,
                                                                                  rel->rd_rel,
         */
        if (!vacuum_is_permitted_for_relation(priv_relid,
                                                                                  rel->rd_rel,

-                                                                                 params.options & ~VACOPT_ANALYZE))
+                                                                                 params.options & ~VACOPT_ANALYZE,
+                                                                                 false))

        {
                relation_close(rel, lmode);
                PopActiveSnapshot();
        {
                relation_close(rel, lmode);
                PopActiveSnapshot();

diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 956d9cea36da6b9a4011286efce6fa3b6d3dcf2a..e62f23748dc6e7b3c836870e88d6f18408d4dbd1 100644 (file)
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -389,7 +389,7 @@ extern bool vacuum_xid_failsafe_check(const struct VacuumCutoffs *cutoffs);
 extern void vac_update_datfrozenxid(void);
 extern void vacuum_delay_point(bool is_analyze);
 extern bool vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple,
 extern void vac_update_datfrozenxid(void);
 extern void vacuum_delay_point(bool is_analyze);
 extern bool vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple,

-                                                                                        uint32 options);
+                                                                                        uint32 options, bool missing_ok);

 extern Relation vacuum_open_relation(Oid relid, RangeVar *relation,
                                                                         uint32 options, bool verbose,
                                                                         LOCKMODE lmode);
 extern Relation vacuum_open_relation(Oid relid, RangeVar *relation,
                                                                         uint32 options, bool verbose,
                                                                         LOCKMODE lmode);