Lower lock level for renaming indexes

Change lock level for renaming index (either ALTER INDEX or implicitly
via some other commands) from AccessExclusiveLock to
ShareUpdateExclusiveLock.

One reason we need a strong lock for relation renaming is that the
name change causes a rebuild of the relcache entry.  Concurrent
sessions that have the relation open might not be able to handle the
relcache entry changing underneath them.  Therefore, we need to lock
the relation in a way that no one can have the relation open
concurrently.  But for indexes, the relcache handles reloads specially
in RelationReloadIndexInfo() in a way that keeps changes in the
relcache entry to a minimum.  As long as no one keeps pointers to
rd_amcache and rd_options around across possible relcache flushes,
which is the case, this ought to be safe.

We also want to use a self-exclusive lock for correctness, so that
concurrent DDL doesn't overwrite the rename if they start updating
while still seeing the old version.  Therefore, we use
ShareUpdateExclusiveLock, which is already used by other DDL commands
that want to operate in a concurrent manner.

The reason this is interesting at all is that renaming an index is a
typical part of a concurrent reindexing workflow (CREATE INDEX
CONCURRENTLY new + DROP INDEX CONCURRENTLY old + rename back).  And
indeed a future built-in REINDEX CONCURRENTLY might rely on the ability
to do concurrent renames as well.

Reviewed-by: Andrey Klychkov <aaklychkov@mail.ru>
Reviewed-by: Fabrízio de Royes Mello <fabriziomello@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/1531767486.432607658@f357.i.mail.ru

diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml
index 73934e5cf37df9797210f41cc2ff775ff61b78eb..bedd9a008d302adb0ececa39686ae25b9fd77905 100644 (file)
--- a/doc/src/sgml/mvcc.sgml
+++ b/doc/src/sgml/mvcc.sgml
@@ -926,10 +926,10 @@ ERROR:  could not serialize access due to read/write dependencies among transact
         <para>
          Acquired by <command>VACUUM</command> (without <option>FULL</option>),
          <command>ANALYZE</command>, <command>CREATE INDEX CONCURRENTLY</command>,
-         <command>CREATE STATISTICS</command> and
-         <command>ALTER TABLE VALIDATE</command> and other
-         <command>ALTER TABLE</command> variants (for full details see
-         <xref linkend="sql-altertable"/>).
+         <command>CREATE STATISTICS</command>, and certain <command>ALTER
+         INDEX</command> and <command>ALTER TABLE</command> variants (for full
+         details see <xref linkend="sql-alterindex"/> and <xref
+         linkend="sql-altertable"/>).
         </para>
        </listitem>
       </varlistentry>
@@ -970,7 +970,7 @@ ERROR:  could not serialize access due to read/write dependencies among transact
         </para>
 
         <para>
-         Acquired by <command>CREATE TRIGGER</command> and many forms of
+         Acquired by <command>CREATE TRIGGER</command> and some forms of
          <command>ALTER TABLE</command> (see <xref linkend="sql-altertable"/>).
         </para>
        </listitem>
@@ -1020,7 +1020,7 @@ ERROR:  could not serialize access due to read/write dependencies among transact
          <command>CLUSTER</command>, <command>VACUUM FULL</command>,
          and <command>REFRESH MATERIALIZED VIEW</command> (without
          <option>CONCURRENTLY</option>)
-         commands. Many forms of <command>ALTER TABLE</command> also acquire
+         commands. Many forms of <command>ALTER INDEX</command> and <command>ALTER TABLE</command> also acquire
          a lock at this level. This is also the default lock mode for
          <command>LOCK TABLE</command> statements that do not specify
          a mode explicitly.

diff --git a/doc/src/sgml/ref/alter_index.sgml b/doc/src/sgml/ref/alter_index.sgml
index d0a62123583fda93c85e7a179bce8bce959932d7..6d34dbb74e5b860b63705144e6ae975ea58c184e 100644 (file)
--- a/doc/src/sgml/ref/alter_index.sgml
+++ b/doc/src/sgml/ref/alter_index.sgml
@@ -39,7 +39,10 @@ ALTER INDEX ALL IN TABLESPACE <replaceable class="parameter">name</replaceable>
 
   <para>
    <command>ALTER INDEX</command> changes the definition of an existing index.
-   There are several subforms:
+   There are several subforms described below. Note that the lock level required
+   may differ for each subform. An <literal>ACCESS EXCLUSIVE</literal> lock is held
+   unless explicitly noted. When multiple subcommands are listed, the lock
+   held will be the strictest one required from any subcommand.
 
   <variablelist>
 
@@ -53,6 +56,10 @@ ALTER INDEX ALL IN TABLESPACE <replaceable class="parameter">name</replaceable>
       or <literal>EXCLUDE</literal>), the constraint is renamed as well.
       There is no effect on the stored data.
      </para>
+     <para>
+      Renaming an index acquires a <literal>SHARE UPDATE EXCLUSIVE</literal>
+      lock.
+     </para>
     </listitem>
    </varlistentry>
 

diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 68be4709771487a91ced14a0f23df9968d847fbd..5ecd2565b4d870f003ac4b4ead6ce6954ad484cc 100644 (file)
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -1661,14 +1661,14 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
                        snprintf(NewToastName, NAMEDATALEN, "pg_toast_%u",
                                         OIDOldHeap);
                        RenameRelationInternal(newrel->rd_rel->reltoastrelid,
-                                                                  NewToastName, true);
+                                                                  NewToastName, true, false);
 
                        /* ... and its valid index too. */
                        snprintf(NewToastName, NAMEDATALEN, "pg_toast_%u_index",
                                         OIDOldHeap);
 
                        RenameRelationInternal(toastidx,
-                                                                  NewToastName, true);
+                                                                  NewToastName, true, true);
                }
                relation_close(newrel, NoLock);
        }

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 946119fa860c0ffcf409b650bbb6764b63d8766d..73da6c39c22bf1aa9aa6e19bd386e882f4375bb1 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -3044,7 +3044,7 @@ rename_constraint_internal(Oid myrelid,
                        || con->contype == CONSTRAINT_UNIQUE
                        || con->contype == CONSTRAINT_EXCLUSION))
                /* rename the index; this renames the constraint as well */
-               RenameRelationInternal(con->conindid, newconname, false);
+               RenameRelationInternal(con->conindid, newconname, false, true);
        else
                RenameConstraintById(constraintOid, newconname);
 
@@ -3112,6 +3112,7 @@ RenameConstraint(RenameStmt *stmt)
 ObjectAddress
 RenameRelation(RenameStmt *stmt)
 {
+       bool            is_index = stmt->renameType == OBJECT_INDEX;
        Oid                     relid;
        ObjectAddress address;
 
@@ -3123,7 +3124,8 @@ RenameRelation(RenameStmt *stmt)
         * Lock level used here should match RenameRelationInternal, to avoid lock
         * escalation.
         */
-       relid = RangeVarGetRelidExtended(stmt->relation, AccessExclusiveLock,
+       relid = RangeVarGetRelidExtended(stmt->relation,
+                                                                        is_index ? ShareUpdateExclusiveLock : AccessExclusiveLock,
                                                                         stmt->missing_ok ? RVR_MISSING_OK : 0,
                                                                         RangeVarCallbackForAlterRelation,
                                                                         (void *) stmt);
@@ -3137,7 +3139,7 @@ RenameRelation(RenameStmt *stmt)
        }
 
        /* Do the work */
-       RenameRelationInternal(relid, stmt->newname, false);
+       RenameRelationInternal(relid, stmt->newname, false, is_index);
 
        ObjectAddressSet(address, RelationRelationId, relid);
 
@@ -3148,7 +3150,7 @@ RenameRelation(RenameStmt *stmt)
  *             RenameRelationInternal - change the name of a relation
  */
 void
-RenameRelationInternal(Oid myrelid, const char *newrelname, bool is_internal)
+RenameRelationInternal(Oid myrelid, const char *newrelname, bool is_internal, bool is_index)
 {
        Relation        targetrelation;
        Relation        relrelation;    /* for RELATION relation */
@@ -3157,11 +3159,16 @@ RenameRelationInternal(Oid myrelid, const char *newrelname, bool is_internal)
        Oid                     namespaceId;
 
        /*
-        * Grab an exclusive lock on the target table, index, sequence, view,
-        * materialized view, or foreign table, which we will NOT release until
-        * end of transaction.
+        * Grab a lock on the target relation, which we will NOT release until end
+        * of transaction.  We need at least a self-exclusive lock so that
+        * concurrent DDL doesn't overwrite the rename if they start updating
+        * while still seeing the old version.  The lock also guards against
+        * triggering relcache reloads in concurrent sessions, which might not
+        * handle this information changing under them.  For indexes, we can use a
+        * reduced lock level because RelationReloadIndexInfo() handles indexes
+        * specially.
         */
-       targetrelation = relation_open(myrelid, AccessExclusiveLock);
+       targetrelation = relation_open(myrelid, is_index ? ShareUpdateExclusiveLock : AccessExclusiveLock);
        namespaceId = RelationGetNamespace(targetrelation);
 
        /*
@@ -3214,7 +3221,7 @@ RenameRelationInternal(Oid myrelid, const char *newrelname, bool is_internal)
        }
 
        /*
-        * Close rel, but keep exclusive lock!
+        * Close rel, but keep lock!
         */
        relation_close(targetrelation, NoLock);
 }
@@ -7076,7 +7083,7 @@ ATExecAddIndexConstraint(AlteredTableInfo *tab, Relation rel,
                ereport(NOTICE,
                                (errmsg("ALTER TABLE / ADD CONSTRAINT USING INDEX will rename index \"%s\" to \"%s\"",
                                                indexName, constraintName)));
-               RenameRelationInternal(index_oid, constraintName, false);
+               RenameRelationInternal(index_oid, constraintName, false, true);
        }
 
        /* Extra checks needed if making primary key */

diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 66f7c577267bbe7e679d41ad6f3e5df7694bf5af..285a0be64368bd0e6aabf7d09ee8330bb57f0b6a 100644 (file)
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -3255,7 +3255,7 @@ RenameType(RenameStmt *stmt)
         * RenameRelationInternal will call RenameTypeInternal automatically.
         */
        if (typTup->typtype == TYPTYPE_COMPOSITE)
-               RenameRelationInternal(typTup->typrelid, newTypeName, false);
+               RenameRelationInternal(typTup->typrelid, newTypeName, false, false);
        else
                RenameTypeInternal(typeOid, newTypeName,
                                                   typTup->typnamespace);

diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index 138de84e832495e21a95af05994dbec39f201b58..2afcd5be4422e2aaeceaa708a4294c5e2ff41f3b 100644 (file)
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -67,7 +67,8 @@ extern ObjectAddress RenameConstraint(RenameStmt *stmt);
 extern ObjectAddress RenameRelation(RenameStmt *stmt);
 
 extern void RenameRelationInternal(Oid myrelid,
-                                          const char *newrelname, bool is_internal);
+                                          const char *newrelname, bool is_internal,
+                                          bool is_index);
 
 extern void find_composite_type_dependencies(Oid typeOid,
                                                                 Relation origRelation,