From 14796959d473784b2768dbf6d9f8ac11934babd4 Mon Sep 17 00:00:00 2001
From: Marti Raudsepp <marti@juffo.org>
Date: Mon, 28 Sep 2015 16:51:45 +0300
Subject: [PATCH] Skip ALTER x SET SCHEMA if the schema didn't change

This is consistent with the behavior of ALTER EXTENSION SET SCHEMA and
many other SET commands. The advantage is that it's now easier to write
DDL scripts that are indempotent.

The object_access_hook hook is still called for each object.
---
 src/backend/catalog/namespace.c             | 19 ++++---------------
 src/backend/commands/alter.c                |  9 ++++++++-
 src/backend/commands/tablecmds.c            | 11 +++++++++--
 src/backend/commands/typecmds.c             | 11 ++++++++++-
 src/include/catalog/namespace.h             |  3 +--
 src/test/regress/expected/alter_generic.out |  1 +
 src/test/regress/expected/alter_table.out   |  5 +++--
 src/test/regress/sql/alter_generic.sql      |  1 +
 src/test/regress/sql/alter_table.sql        |  4 +++-
 9 files changed, 40 insertions(+), 24 deletions(-)

diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index b16af64..8cf8b17 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -2769,24 +2769,13 @@ LookupCreationNamespace(const char *nspname)
 /*
  * Common checks on switching namespaces.
  *
- * We complain if (1) the old and new namespaces are the same, (2) either the
- * old or new namespaces is a temporary schema (or temporary toast schema), or
- * (3) either the old or new namespaces is the TOAST schema.
+ * We complain if (1) either the old or new namespaces is a temporary schema
+ * (or temporary toast schema), or (3) either the old or new namespaces is the
+ * TOAST schema.
  */
 void
-CheckSetNamespace(Oid oldNspOid, Oid nspOid, Oid classid, Oid objid)
+CheckSetNamespace(Oid oldNspOid, Oid nspOid)
 {
-	if (oldNspOid == nspOid)
-		ereport(ERROR,
-				(classid == RelationRelationId ?
-				 errcode(ERRCODE_DUPLICATE_TABLE) :
-				 classid == ProcedureRelationId ?
-				 errcode(ERRCODE_DUPLICATE_FUNCTION) :
-				 errcode(ERRCODE_DUPLICATE_OBJECT),
-				 errmsg("%s is already in schema \"%s\"",
-						getObjectDescriptionOids(classid, objid),
-						get_namespace_name(nspOid))));
-
 	/* disallow renaming into or out of temp schemas */
 	if (isAnyTempNamespace(nspOid) || isAnyTempNamespace(oldNspOid))
 		ereport(ERROR,
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index d28758c..470d26c 100644
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -592,8 +592,15 @@ AlterObjectNamespace_internal(Relation rel, Oid objid, Oid nspOid)
 	Assert(!isnull);
 	oldNspOid = DatumGetObjectId(namespace);
 
+	/* The object is already there, nothing to do */
+	if (oldNspOid == nspOid)
+	{
+		InvokeObjectPostAlterHook(classId, objid, 0);
+		return oldNspOid;
+	}
+
 	/* Check basic namespace related issues */
-	CheckSetNamespace(oldNspOid, nspOid, classId, objid);
+	CheckSetNamespace(oldNspOid, nspOid);
 
 	/* Permission checks ... superusers can always do it */
 	if (!superuser())
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 126b119..2a8313a 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -11310,7 +11310,7 @@ AlterTableNamespace(AlterObjectSchemaStmt *stmt, Oid *oldschema)
 	nspOid = RangeVarGetAndCheckCreationNamespace(newrv, NoLock, NULL);
 
 	/* common checks on switching namespaces */
-	CheckSetNamespace(oldNspOid, nspOid, RelationRelationId, relid);
+	CheckSetNamespace(oldNspOid, nspOid);
 
 	objsMoved = new_object_addresses();
 	AlterTableNamespaceInternal(rel, oldNspOid, nspOid, objsMoved);
@@ -11378,6 +11378,7 @@ AlterRelationNamespaceInternal(Relation classRel, Oid relOid,
 	HeapTuple	classTup;
 	Form_pg_class classForm;
 	ObjectAddress thisobj;
+	bool 		alreadyChanged;
 
 	classTup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(relOid));
 	if (!HeapTupleIsValid(classTup))
@@ -11390,10 +11391,12 @@ AlterRelationNamespaceInternal(Relation classRel, Oid relOid,
 	thisobj.objectId = relOid;
 	thisobj.objectSubId = 0;
 
+	alreadyChanged = object_address_present(&thisobj, objsMoved);
+
 	/*
 	 * Do nothing when there's nothing to do.
 	 */
-	if (!object_address_present(&thisobj, objsMoved))
+	if (!alreadyChanged && oldNspOid != newNspOid)
 	{
 		/* check for duplicate name (more friendly than unique-index failure) */
 		if (get_relname_relid(NameStr(classForm->relname),
@@ -11419,7 +11422,11 @@ AlterRelationNamespaceInternal(Relation classRel, Oid relOid,
 								newNspOid) != 1)
 			elog(ERROR, "failed to change schema dependency for relation \"%s\"",
 				 NameStr(classForm->relname));
+	}
 
+	/* Hooks are still fired even if namespace didn't change */
+	if (!alreadyChanged)
+	{
 		add_exact_object_address(&thisobj, objsMoved);
 
 		InvokeObjectPostAlterHook(RelationRelationId, relOid, 0);
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index d2b3f22..5b6f6c7 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -3520,8 +3520,17 @@ AlterTypeNamespaceInternal(Oid typeOid, Oid nspOid,
 	oldNspOid = typform->typnamespace;
 	arrayOid = typform->typarray;
 
+	/* The type is already there, nothing to do */
+	if (oldNspOid == nspOid)
+	{
+		InvokeObjectPostAlterHook(TypeRelationId, typeOid, 0);
+		heap_freetuple(tup);
+		heap_close(rel, RowExclusiveLock);
+		return oldNspOid;
+	}
+
 	/* common checks on switching namespaces */
-	CheckSetNamespace(oldNspOid, nspOid, TypeRelationId, typeOid);
+	CheckSetNamespace(oldNspOid, nspOid);
 
 	/* check for duplicate name (more friendly than unique-index failure) */
 	if (SearchSysCacheExists2(TYPENAMENSP,
diff --git a/src/include/catalog/namespace.h b/src/include/catalog/namespace.h
index f3b005f..b6ad934 100644
--- a/src/include/catalog/namespace.h
+++ b/src/include/catalog/namespace.h
@@ -112,8 +112,7 @@ extern Oid	LookupExplicitNamespace(const char *nspname, bool missing_ok);
 extern Oid	get_namespace_oid(const char *nspname, bool missing_ok);
 
 extern Oid	LookupCreationNamespace(const char *nspname);
-extern void CheckSetNamespace(Oid oldNspOid, Oid nspOid, Oid classid,
-				  Oid objid);
+extern void CheckSetNamespace(Oid oldNspOid, Oid nspOid);
 extern Oid	QualifiedNameGetCreationNamespace(List *names, char **objname_p);
 extern RangeVar *makeRangeVarFromNameList(List *names);
 extern char *NameListToString(List *names);
diff --git a/src/test/regress/expected/alter_generic.out b/src/test/regress/expected/alter_generic.out
index 4c3c882..43376ee 100644
--- a/src/test/regress/expected/alter_generic.out
+++ b/src/test/regress/expected/alter_generic.out
@@ -40,6 +40,7 @@ ALTER FUNCTION alt_func1(int) RENAME TO alt_func3;  -- OK
 ALTER FUNCTION alt_func2(int) OWNER TO regtest_alter_user2;  -- failed (no role membership)
 ERROR:  must be member of role "regtest_alter_user2"
 ALTER FUNCTION alt_func2(int) OWNER TO regtest_alter_user3;  -- OK
+ALTER FUNCTION alt_func2(int) SET SCHEMA alt_nsp1;  -- OK, already there
 ALTER FUNCTION alt_func2(int) SET SCHEMA alt_nsp2;  -- OK
 ALTER AGGREGATE alt_agg1(int) RENAME TO alt_agg2;   -- failed (name conflict)
 ERROR:  function alt_agg2(integer) already exists in schema "alt_nsp1"
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 28422ea..aa13ff2 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -2120,6 +2120,7 @@ create text search template alter1.tmpl(init = dsimple_init, lexize = dsimple_le
 create text search dictionary alter1.dict(template = alter1.tmpl);
 insert into alter1.t1(f2) values(11);
 insert into alter1.t1(f2) values(12);
+alter table alter1.t1 set schema alter1; -- no-op, same schema
 alter table alter1.t1 set schema alter2;
 alter table alter1.v1 set schema alter2;
 alter function alter1.plus1(int) set schema alter2;
@@ -2128,6 +2129,7 @@ alter operator class alter1.ctype_hash_ops using hash set schema alter2;
 alter operator family alter1.ctype_hash_ops using hash set schema alter2;
 alter operator alter1.=(alter1.ctype, alter1.ctype) set schema alter2;
 alter function alter1.same(alter1.ctype, alter1.ctype) set schema alter2;
+alter type alter1.ctype set schema alter1; -- no-op, same schema
 alter type alter1.ctype set schema alter2;
 alter conversion alter1.ascii_to_utf8 set schema alter2;
 alter text search parser alter1.prs set schema alter2;
@@ -2554,9 +2556,8 @@ ALTER TABLE new_system_table SET SCHEMA pg_catalog;
 -- XXX: it's currently impossible to move relations out of pg_catalog
 ALTER TABLE new_system_table SET SCHEMA public;
 ERROR:  cannot remove dependency on schema pg_catalog because it is a system object
--- move back, will currently error out, already there
+-- move back, will be ignored -- already there
 ALTER TABLE new_system_table SET SCHEMA pg_catalog;
-ERROR:  table new_system_table is already in schema "pg_catalog"
 ALTER TABLE new_system_table RENAME TO old_system_table;
 CREATE INDEX old_system_table__othercol ON old_system_table (othercol);
 INSERT INTO old_system_table(othercol) VALUES ('somedata'), ('otherdata');
diff --git a/src/test/regress/sql/alter_generic.sql b/src/test/regress/sql/alter_generic.sql
index ed4398b..8a811d4 100644
--- a/src/test/regress/sql/alter_generic.sql
+++ b/src/test/regress/sql/alter_generic.sql
@@ -44,6 +44,7 @@ ALTER FUNCTION alt_func1(int) RENAME TO alt_func2;  -- failed (name conflict)
 ALTER FUNCTION alt_func1(int) RENAME TO alt_func3;  -- OK
 ALTER FUNCTION alt_func2(int) OWNER TO regtest_alter_user2;  -- failed (no role membership)
 ALTER FUNCTION alt_func2(int) OWNER TO regtest_alter_user3;  -- OK
+ALTER FUNCTION alt_func2(int) SET SCHEMA alt_nsp1;  -- OK, already there
 ALTER FUNCTION alt_func2(int) SET SCHEMA alt_nsp2;  -- OK
 
 ALTER AGGREGATE alt_agg1(int) RENAME TO alt_agg2;   -- failed (name conflict)
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 3ef55d9..abf50e8 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1461,6 +1461,7 @@ create text search dictionary alter1.dict(template = alter1.tmpl);
 insert into alter1.t1(f2) values(11);
 insert into alter1.t1(f2) values(12);
 
+alter table alter1.t1 set schema alter1; -- no-op, same schema
 alter table alter1.t1 set schema alter2;
 alter table alter1.v1 set schema alter2;
 alter function alter1.plus1(int) set schema alter2;
@@ -1469,6 +1470,7 @@ alter operator class alter1.ctype_hash_ops using hash set schema alter2;
 alter operator family alter1.ctype_hash_ops using hash set schema alter2;
 alter operator alter1.=(alter1.ctype, alter1.ctype) set schema alter2;
 alter function alter1.same(alter1.ctype, alter1.ctype) set schema alter2;
+alter type alter1.ctype set schema alter1; -- no-op, same schema
 alter type alter1.ctype set schema alter2;
 alter conversion alter1.ascii_to_utf8 set schema alter2;
 alter text search parser alter1.prs set schema alter2;
@@ -1700,7 +1702,7 @@ ALTER TABLE new_system_table SET SCHEMA pg_catalog;
 
 -- XXX: it's currently impossible to move relations out of pg_catalog
 ALTER TABLE new_system_table SET SCHEMA public;
--- move back, will currently error out, already there
+-- move back, will be ignored -- already there
 ALTER TABLE new_system_table SET SCHEMA pg_catalog;
 ALTER TABLE new_system_table RENAME TO old_system_table;
 CREATE INDEX old_system_table__othercol ON old_system_table (othercol);
-- 
2.5.3

