Don't lock tables in get_tables_to_repack()

When doing a whole database repack, we build a list of tables to process
taking a lock on each.  But because it's a regular transaction-scoped
lock, it's automatically released immediately after building the list
anyway, which makes it not very useful.  (Also, we have three ways to
obtain a list of tables to repack, and only one of them acquired this
lock.)  Remove that lock acquisition, as it's useless and inconsistent.

We acquire a lock properly afterwards (and recheck that the table can
still be repacked as indicated), so we don't need to do anything other
than drop that initial lock acquisition and harden the code in
repack_is_permitted_for_relation() against possible concurrent drops.
This is similar to how vacuum does it in get_all_vacuum_rels().

In order for this to work reliably, also change
repack_is_permitted_for_relation() to cope with the possibility of the
table going away partway through.  Similarly, in ExecRepack(), be
prepared for what we believed to be a table or matview to now be
something else, and skip it without erroring out, by changing
try_table_open() to try_relation_open() and testing the relkind
separately.

While at it, replace one relation_close() call in get_tables_to_repack()
with table_close() to match the table_open() that opened the catalog.

Author: ChangAo Chen <cca5507@qq.com>
Backpatch-through: 19
Discussion: https://postgr.es/m/tencent_9F290B256A3F52B66542F1140E32ECC64309@qq.com

diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index faa07d1a118b049e66299eeaa199b495f5b451da..02883fe34a4855f953cccd9c6051898938c3be4d 100644 (file)
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -375,7 +375,8 @@ ExecRepack(ParseState *pstate, RepackStmt *stmt, bool isTopLevel)
        /*
         * If we don't have a relation yet, determine a relation list.  If we do,
         * then it must be a partitioned table, and we want to process its
-        * partitions.
+        * partitions.  Note that we don't acquire any locks on these tables, so
+        * the returned list must be treated with suspicion.
         */
        if (rel == NULL)
        {
@@ -452,15 +453,22 @@ ExecRepack(ParseState *pstate, RepackStmt *stmt, bool isTopLevel)
                StartTransactionCommand();
 
                /*
-                * Open the target table, coping with the case where it has been
-                * dropped.
+                * Open the target table.  It may have been dropped or replaced with
+                * something different, in which case silently skip it.
                 */
-               rel = try_table_open(rtc->tableOid, lockmode);
+               rel = try_relation_open(rtc->tableOid, lockmode);
                if (rel == NULL)
                {
                        CommitTransactionCommand();
                        continue;
                }
+               if (rel->rd_rel->relkind != RELKIND_RELATION &&
+                       rel->rd_rel->relkind != RELKIND_MATVIEW)
+               {
+                       relation_close(rel, lockmode);
+                       CommitTransactionCommand();
+                       continue;
+               }
 
                /* functions in indexes may want a snapshot set */
                PushActiveSnapshot(GetTransactionSnapshot());
@@ -713,6 +721,8 @@ cluster_rel_recheck(RepackCommand cmd, Relation OldHeap, Oid indexOid,
 {
        Oid                     tableOid = RelationGetRelid(OldHeap);
 
+       Assert(CheckRelationLockedByMe(OldHeap, lmode, false));
+
        /* Check that the user still has privileges for the relation */
        if (!repack_is_permitted_for_relation(cmd, tableOid, userid))
        {
@@ -2152,6 +2162,10 @@ get_tables_to_repack(RepackCommand cmd, bool usingindex, MemoryContext permcxt)
 
                /*
                 * For USING INDEX, scan pg_index to find those with indisclustered.
+                *
+                * Note we don't obtain lock of any kind on the index, which means the
+                * index or its owning table could be gone or change at any point.  We
+                * have to be extra careful when examining catalog state for them.
                 */
                catalog = table_open(IndexRelationId, AccessShareLock);
                ScanKeyInit(&entry,
@@ -2164,47 +2178,28 @@ get_tables_to_repack(RepackCommand cmd, bool usingindex, MemoryContext permcxt)
                        RelToCluster *rtc;
                        Form_pg_index index;
                        HeapTuple       classtup;
-                       Form_pg_class classForm;
+                       Oid                     relnamespace;
+                       char            relpersistence;
                        MemoryContext oldcxt;
 
                        index = (Form_pg_index) GETSTRUCT(tuple);
 
-                       /*
-                        * Try to obtain a light lock on the index's table, to ensure it
-                        * doesn't go away while we collect the list.  If we cannot, just
-                        * disregard it.  Be sure to release this if we ultimately decide
-                        * not to process the table!
-                        */
-                       if (!ConditionalLockRelationOid(index->indrelid, AccessShareLock))
-                               continue;
-
-                       /* Verify that the table still exists; skip if not */
                        classtup = SearchSysCache1(RELOID, ObjectIdGetDatum(index->indrelid));
                        if (!HeapTupleIsValid(classtup))
-                       {
-                               UnlockRelationOid(index->indrelid, AccessShareLock);
                                continue;
-                       }
-                       classForm = (Form_pg_class) GETSTRUCT(classtup);
+                       relnamespace = ((Form_pg_class) GETSTRUCT(classtup))->relnamespace;
+                       relpersistence = ((Form_pg_class) GETSTRUCT(classtup))->relpersistence;
+                       ReleaseSysCache(classtup);
 
                        /* Skip temp relations belonging to other sessions */
-                       if (classForm->relpersistence == RELPERSISTENCE_TEMP &&
-                               !isTempOrTempToastNamespace(classForm->relnamespace))
-                       {
-                               ReleaseSysCache(classtup);
-                               UnlockRelationOid(index->indrelid, AccessShareLock);
+                       if (relpersistence == RELPERSISTENCE_TEMP &&
+                               !isTempOrTempToastNamespace(relnamespace))
                                continue;
-                       }
-
-                       ReleaseSysCache(classtup);
 
                        /* noisily skip rels which the user can't process */
                        if (!repack_is_permitted_for_relation(cmd, index->indrelid,
                                                                                                  GetUserId()))
-                       {
-                               UnlockRelationOid(index->indrelid, AccessShareLock);
                                continue;
-                       }
 
                        /* Use a permanent memory context for the result list */
                        oldcxt = MemoryContextSwitchTo(permcxt);
@@ -2228,45 +2223,20 @@ get_tables_to_repack(RepackCommand cmd, bool usingindex, MemoryContext permcxt)
 
                        class = (Form_pg_class) GETSTRUCT(tuple);
 
-                       /*
-                        * Try to obtain a light lock on the table, to ensure it doesn't
-                        * go away while we collect the list.  If we cannot, just
-                        * disregard the table.  Be sure to release this if we ultimately
-                        * decide not to process the table!
-                        */
-                       if (!ConditionalLockRelationOid(class->oid, AccessShareLock))
-                               continue;
-
-                       /* Verify that the table still exists */
-                       if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(class->oid)))
-                       {
-                               UnlockRelationOid(class->oid, AccessShareLock);
-                               continue;
-                       }
-
                        /* Can only process plain tables and matviews */
                        if (class->relkind != RELKIND_RELATION &&
                                class->relkind != RELKIND_MATVIEW)
-                       {
-                               UnlockRelationOid(class->oid, AccessShareLock);
                                continue;
-                       }
 
                        /* Skip temp relations belonging to other sessions */
                        if (class->relpersistence == RELPERSISTENCE_TEMP &&
                                !isTempOrTempToastNamespace(class->relnamespace))
-                       {
-                               UnlockRelationOid(class->oid, AccessShareLock);
                                continue;
-                       }
 
                        /* noisily skip rels which the user can't process */
                        if (!repack_is_permitted_for_relation(cmd, class->oid,
                                                                                                  GetUserId()))
-                       {
-                               UnlockRelationOid(class->oid, AccessShareLock);
                                continue;
-                       }
 
                        /* Use a permanent memory context for the result list */
                        oldcxt = MemoryContextSwitchTo(permcxt);
@@ -2279,7 +2249,7 @@ get_tables_to_repack(RepackCommand cmd, bool usingindex, MemoryContext permcxt)
        }
 
        table_endscan(scan);
-       relation_close(catalog, AccessShareLock);
+       table_close(catalog, AccessShareLock);
 
        return rtcs;
 }
@@ -2351,21 +2321,42 @@ get_tables_to_repack_partitioned(RepackCommand cmd, Oid relid,
 
 
 /*
- * Return whether userid has privileges to REPACK relid.  If not, this
- * function emits a WARNING.
+ * Return whether userid has privileges to execute REPACK on relid.
+ *
+ * Caller may not have a lock on the relation, so it could have been
+ * dropped concurrently.  In that case, silently return false.
+ *
+ * If the relation does exist but the user doesn't have the required
+ * privs, emit a WARNING and return false.  Otherwise, return true.
  */
 static bool
 repack_is_permitted_for_relation(RepackCommand cmd, Oid relid, Oid userid)
 {
+       bool            is_missing = false;
+       AclResult       result;
+       char       *relname;
+
        Assert(cmd == REPACK_COMMAND_CLUSTER || cmd == REPACK_COMMAND_REPACK);
 
-       if (pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK)
+       result = pg_class_aclcheck_ext(relid, userid, ACL_MAINTAIN, &is_missing);
+       if (is_missing)
+               return false;
+
+       if (result == ACLCHECK_OK)
                return true;
 
-       ereport(WARNING,
-                       errmsg("permission denied to execute %s on \"%s\", skipping it",
-                                  RepackCommandAsString(cmd),
-                                  get_rel_name(relid)));
+       /*
+        * The relation can also be dropped after we tested its ACL and before we
+        * read its relname, so be careful here.
+        */
+       relname = get_rel_name(relid);
+       if (relname != NULL)
+       {
+               ereport(WARNING,
+                               errmsg("permission denied to execute %s on \"%s\", skipping it",
+                                          RepackCommandAsString(cmd), relname));
+               pfree(relname);
+       }
 
        return false;
 }