On 03/10/2018 13:51, Andrey Klychkov wrote:
> 1. Patch was applied without any errors except a part related to
> documentation:
> error: patch failed: doc/src/sgml/ref/alter_index.sgml:50
> error: doc/src/sgml/ref/alter_index.sgml: patch does not apply
Attached is an updated patch.
> 2. The code has been compiled successfully, configured by:
> # ./configure CFLAGS="-O0" --enable-debug --enable-cassert
> --enable-depend --without-zlib
Not sure why you use -O0 here. It's not a good idea for development,
because it might miss interesting warnings.
> 7. Code style:
> +RenameRelationInternal(Oid myrelid, const char *newrelname, bool
> is_internal, bool is_index)
> This line is longer than 80 chars.
pgindent leaves this line alone.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 37591a06901e2d694e3928b7e1cddfcfd84f6267 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Mon, 13 Aug 2018 22:38:36 +0200
Subject: [PATCH v2] 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.
The reason we need a strong lock at all 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.
One could perhaps argue that this could all be done with an
AccessShareLock then. But we standardize here for consistency on
ShareUpdateExclusiveLock, which is already used by other DDL commands
that want to operate in a concurrent manner. For example, renaming an
index concurrently with CREATE INDEX CONCURRENTLY might be trouble.
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 | 24 ++++++++++++++----------
src/backend/commands/typecmds.c | 2 +-
src/include/commands/tablecmds.h | 3 ++-
6 files changed, 33 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 @@ <title>Table-level Lock Modes</title>
<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 @@ <title>Table-level Lock Modes</title>
</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 @@ <title>Table-level Lock Modes</title>
<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 d0a6212358..6d34dbb74e 100644
--- a/doc/src/sgml/ref/alter_index.sgml
+++ b/doc/src/sgml/ref/alter_index.sgml
@@ -39,7 +39,10 @@ <title>Description</title>
<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 @@ <title>Description</title>
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 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 c145385f84..b1c1181e55 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,13 @@ 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. The lock here guards mainly 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 +3216,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);
}
@@ -7069,7 +7073,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 b018585aef..6506db80b0 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -3283,7 +3283,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: 803b1301e8c9aac478abeec62824a5d09664ffff
--
2.19.0
