From 3188b76cb4a2851b87361193addb6cbe326d4a64 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Thu, 25 Oct 2018 08:33:17 +0100 Subject: [PATCH v3] 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. --- doc/src/sgml/mvcc.sgml | 12 ++++++------ doc/src/sgml/ref/alter_index.sgml | 9 ++++++++- src/backend/commands/cluster.c | 4 ++-- src/backend/commands/tablecmds.c | 27 +++++++++++++++++---------- src/backend/commands/typecmds.c | 2 +- src/include/commands/tablecmds.h | 3 ++- 6 files changed, 36 insertions(+), 21 deletions(-) diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml index 73934e5cf3..bedd9a008d 100644 --- a/doc/src/sgml/mvcc.sgml +++ b/doc/src/sgml/mvcc.sgml @@ -926,10 +926,10 @@ Table-level Lock Modes Acquired by VACUUM (without ), ANALYZE, CREATE INDEX CONCURRENTLY, - CREATE STATISTICS and - ALTER TABLE VALIDATE and other - ALTER TABLE variants (for full details see - ). + CREATE STATISTICS, and certain ALTER + INDEX and ALTER TABLE variants (for full + details see and ). @@ -970,7 +970,7 @@ Table-level Lock Modes - Acquired by CREATE TRIGGER and many forms of + Acquired by CREATE TRIGGER and some forms of ALTER TABLE (see ). @@ -1020,7 +1020,7 @@ Table-level Lock Modes CLUSTER, VACUUM FULL, and REFRESH MATERIALIZED VIEW (without ) - commands. Many forms of ALTER TABLE also acquire + commands. Many forms of ALTER INDEX and ALTER TABLE also acquire a lock at this level. This is also the default lock mode for LOCK TABLE 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 d0a6212358..6d34dbb74e 100644 --- a/doc/src/sgml/ref/alter_index.sgml +++ b/doc/src/sgml/ref/alter_index.sgml @@ -39,7 +39,10 @@ Description ALTER INDEX 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 ACCESS EXCLUSIVE lock is held + unless explicitly noted. When multiple subcommands are listed, the lock + held will be the strictest one required from any subcommand. @@ -53,6 +56,10 @@ Description or EXCLUDE), the constraint is renamed as well. There is no effect on the stored data. + + Renaming an index acquires a SHARE UPDATE EXCLUSIVE + lock. + diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 68be470977..5ecd2565b4 100644 --- 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 153aec263e..0a7f4a587e 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -3042,7 +3042,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); @@ -3110,6 +3110,7 @@ RenameConstraint(RenameStmt *stmt) ObjectAddress RenameRelation(RenameStmt *stmt) { + bool is_index = stmt->renameType == OBJECT_INDEX; Oid relid; ObjectAddress address; @@ -3121,7 +3122,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); @@ -3135,7 +3137,7 @@ RenameRelation(RenameStmt *stmt) } /* Do the work */ - RenameRelationInternal(relid, stmt->newname, false); + RenameRelationInternal(relid, stmt->newname, false, is_index); ObjectAddressSet(address, RelationRelationId, relid); @@ -3146,7 +3148,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 */ @@ -3155,11 +3157,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); /* @@ -3212,7 +3219,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); } @@ -7070,7 +7077,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 3271962a7a..69429a8b62 100644 --- a/src/backend/commands/typecmds.c +++ b/src/backend/commands/typecmds.c @@ -3262,7 +3262,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 138de84e83..2afcd5be44 100644 --- 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, base-commit: 10074651e3355e2405015f6253602be8344bc829 -- 2.19.1