[PATCH] Skip ALTER x SET SCHEMA if the schema didn't change
Hi list
The attached patch changes the behavior of multiple ALTER x SET SCHEMA
commands, to skip, rather than fail, when the old and new schema is
the same.
The advantage is that it's now easier to write DDL scripts that are indempotent.
This already matches the behavior of ALTER EXTENSION SET SCHEMA in
earlier versions, as well as many other SET-ish commands, e.g. ALTER
TABLE SET TABLESPACE, OWNER TO, CLUSTER ON, SET (storage_parameter...)
etc. I don't see why SET SCHEMA should be treated any differently.
The code is written such that object_access_hook is still called for
each object.
Regression tests included. I couldn't find any documentation that
needs changing.
Regards,
Marti
Attachments:
0001-Skip-ALTER-x-SET-SCHEMA-if-the-schema-didn-t-change.patchbinary/octet-stream; name=0001-Skip-ALTER-x-SET-SCHEMA-if-the-schema-didn-t-change.patchDownload
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
On Tue, Sep 29, 2015 at 12:17 AM, Marti Raudsepp <marti@juffo.org> wrote:
Hi list
The attached patch changes the behavior of multiple ALTER x SET SCHEMA
commands, to skip, rather than fail, when the old and new schema is
the same.The advantage is that it's now easier to write DDL scripts that are indempotent.
This already matches the behavior of ALTER EXTENSION SET SCHEMA in
earlier versions, as well as many other SET-ish commands, e.g. ALTER
TABLE SET TABLESPACE, OWNER TO, CLUSTER ON, SET (storage_parameter...)
etc. I don't see why SET SCHEMA should be treated any differently.The code is written such that object_access_hook is still called for
each object.Regression tests included. I couldn't find any documentation that
needs changing.
I went through the patch, following are my observations,
Patch applied with hunks and compiled with out warnings.
Basic tests are passed.
In AlterTableNamespaceInternal function, if a table or matview called
for set schema,
If the object contains any constraints, the constraint gets updated
with new schema.
In AlterTypeNamespaceInternal function, the InvokeObjectPostAlterHook function
doesn't get called if the type is of composite type, domain and array
types as because
it just returns from top of the function.
Regards,
Hari Babu
Fujitsu Australia
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Nov 5, 2015 at 6:20 AM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
I went through the patch, following are my observations,
Patch applied with hunks and compiled with out warnings.
Basic tests are passed.
I'm interested in hearing opinions from multiple people about the
following two questions:
1. Is the new behavior better than the old behavior?
2. Will breaking backward compatibility make too many people unhappy?
My guess is that the answer to the first question is "yes" and that
the answer to the second one is "no", but this is clearly a
significant incompatibility, so I'd like to hear some more opinions
before concluding that we definitely want to do this.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Nov 5, 2015 at 2:15 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Nov 5, 2015 at 6:20 AM, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:I went through the patch, following are my observations,
Patch applied with hunks and compiled with out warnings.
Basic tests are passed.I'm interested in hearing opinions from multiple people about the
following two questions:1. Is the new behavior better than the old behavior?
2. Will breaking backward compatibility make too many people unhappy?My guess is that the answer to the first question is "yes" and that
the answer to the second one is "no", but this is clearly a
significant incompatibility, so I'd like to hear some more opinions
before concluding that we definitely want to do this.
For #2 I'm not that concerned about turning an error case into a non-error.
The rationale behind #1 makes sense to me. Given all the recent work on
"IF NOT EXISTS" we obviously think that this general behavior is desirable
and we should correct this deviation from that norm.
David J.
David G. Johnston wrote:
On Thu, Nov 5, 2015 at 2:15 PM, Robert Haas <robertmhaas@gmail.com> wrote:
I'm interested in hearing opinions from multiple people about the
following two questions:1. Is the new behavior better than the old behavior?
2. Will breaking backward compatibility make too many people unhappy?My guess is that the answer to the first question is "yes" and that
the answer to the second one is "no", but this is clearly a
significant incompatibility, so I'd like to hear some more opinions
before concluding that we definitely want to do this.For #2 I'm not that concerned about turning an error case into a non-error.
Yeah, maybe I lack imagination but I don't see how the current behavior
is actually useful. We already silently do nothing when appropriate in
many other DDL commands.
The rationale behind #1 makes sense to me. Given all the recent work on
"IF NOT EXISTS" we obviously think that this general behavior is desirable
and we should correct this deviation from that norm.
Agreed.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Nov 5, 2015 at 10:20 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
On Tue, Sep 29, 2015 at 12:17 AM, Marti Raudsepp <marti@juffo.org> wrote:
Hi list
The attached patch changes the behavior of multiple ALTER x SET SCHEMA
commands, to skip, rather than fail, when the old and new schema is
the same.The advantage is that it's now easier to write DDL scripts that are indempotent.
This already matches the behavior of ALTER EXTENSION SET SCHEMA in
earlier versions, as well as many other SET-ish commands, e.g. ALTER
TABLE SET TABLESPACE, OWNER TO, CLUSTER ON, SET (storage_parameter...)
etc. I don't see why SET SCHEMA should be treated any differently.The code is written such that object_access_hook is still called for
each object.Regression tests included. I couldn't find any documentation that
needs changing.I went through the patch, following are my observations,
Patch applied with hunks and compiled with out warnings.
Basic tests are passed.In AlterTableNamespaceInternal function, if a table or matview called
for set schema,
If the object contains any constraints, the constraint gets updated
with new schema.In AlterTypeNamespaceInternal function, the InvokeObjectPostAlterHook function
doesn't get called if the type is of composite type, domain and array
types as because
it just returns from top of the function.
Most of the community members didn't find any problem in changing the
behavior, so here I attached updated patch with the above two corrections.
Regards,
Hari Babu
Fujitsu Australia
Attachments:
0001-Skip-ALTER-x-SET-SCHEMA-if-the-schema-didn-t-change_v2.patchapplication/octet-stream; name=0001-Skip-ALTER-x-SET-SCHEMA-if-the-schema-didn-t-change_v2.patchDownload
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/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index 3c756f8..c48a3c3 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -718,15 +718,17 @@ AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
{
Form_pg_constraint conform = (Form_pg_constraint) GETSTRUCT(tup);
ObjectAddress thisobj;
+ bool alreadyChanged;
thisobj.classId = ConstraintRelationId;
thisobj.objectId = HeapTupleGetOid(tup);
thisobj.objectSubId = 0;
- if (object_address_present(&thisobj, objsMoved))
- continue;
-
- if (conform->connamespace == oldNspId)
+ alreadyChanged = object_address_present(&thisobj, objsMoved);
+
+ /* Don't update if the object is already part of the namespace */
+ if (!alreadyChanged && conform->connamespace == oldNspId
+ && oldNspId != newNspId)
{
tup = heap_copytuple(tup);
conform = (Form_pg_constraint) GETSTRUCT(tup);
@@ -743,9 +745,12 @@ AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
*/
}
- InvokeObjectPostAlterHook(ConstraintRelationId, thisobj.objectId, 0);
+ if (!alreadyChanged)
+ {
+ InvokeObjectPostAlterHook(ConstraintRelationId, thisobj.objectId, 0);
- add_exact_object_address(&thisobj, objsMoved);
+ add_exact_object_address(&thisobj, objsMoved);
+ }
}
systable_endscan(scan);
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 55ab209..0a036e1 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -11356,7 +11356,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);
@@ -11424,6 +11424,7 @@ AlterRelationNamespaceInternal(Relation classRel, Oid relOid,
HeapTuple classTup;
Form_pg_class classForm;
ObjectAddress thisobj;
+ bool alreadyChanged;
classTup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(relOid));
if (!HeapTupleIsValid(classTup))
@@ -11436,10 +11437,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),
@@ -11465,7 +11468,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..434bd8f 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -3520,18 +3520,22 @@ AlterTypeNamespaceInternal(Oid typeOid, Oid nspOid,
oldNspOid = typform->typnamespace;
arrayOid = typform->typarray;
- /* common checks on switching namespaces */
- CheckSetNamespace(oldNspOid, nspOid, TypeRelationId, typeOid);
+ /* The type is already there, nothing to do */
+ if (oldNspOid != nspOid)
+ {
+ /* common checks on switching namespaces */
+ CheckSetNamespace(oldNspOid, nspOid);
- /* check for duplicate name (more friendly than unique-index failure) */
- if (SearchSysCacheExists2(TYPENAMENSP,
- CStringGetDatum(NameStr(typform->typname)),
- ObjectIdGetDatum(nspOid)))
- ereport(ERROR,
- (errcode(ERRCODE_DUPLICATE_OBJECT),
- errmsg("type \"%s\" already exists in schema \"%s\"",
- NameStr(typform->typname),
- get_namespace_name(nspOid))));
+ /* check for duplicate name (more friendly than unique-index failure) */
+ if (SearchSysCacheExists2(TYPENAMENSP,
+ CStringGetDatum(NameStr(typform->typname)),
+ ObjectIdGetDatum(nspOid)))
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("type \"%s\" already exists in schema \"%s\"",
+ NameStr(typform->typname),
+ get_namespace_name(nspOid))));
+ }
/* Detect whether type is a composite type (but not a table rowtype) */
isCompositeType =
@@ -3547,13 +3551,16 @@ AlterTypeNamespaceInternal(Oid typeOid, Oid nspOid,
format_type_be(typeOid)),
errhint("Use ALTER TABLE instead.")));
- /* OK, modify the pg_type row */
+ if (oldNspOid != nspOid)
+ {
+ /* OK, modify the pg_type row */
- /* tup is a copy, so we can scribble directly on it */
- typform->typnamespace = nspOid;
+ /* tup is a copy, so we can scribble directly on it */
+ typform->typnamespace = nspOid;
- simple_heap_update(rel, &tup->t_self, tup);
- CatalogUpdateIndexes(rel, tup);
+ simple_heap_update(rel, &tup->t_self, tup);
+ CatalogUpdateIndexes(rel, tup);
+ }
/*
* Composite types have pg_class entries.
@@ -3592,8 +3599,8 @@ AlterTypeNamespaceInternal(Oid typeOid, Oid nspOid,
* Update dependency on schema, if any --- a table rowtype has not got
* one, and neither does an implicit array.
*/
- if ((isCompositeType || typform->typtype != TYPTYPE_COMPOSITE) &&
- !isImplicitArray)
+ if ((oldNspOid != nspOid) && ((isCompositeType || typform->typtype != TYPTYPE_COMPOSITE)
+ && !isImplicitArray))
if (changeDependencyFor(TypeRelationId, typeOid,
NamespaceRelationId, oldNspOid, nspOid) != 1)
elog(ERROR, "failed to change schema dependency for type %s",
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 44ce6f5..eb57242 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -2133,6 +2133,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;
@@ -2141,6 +2142,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;
@@ -2567,9 +2569,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 778791d..6740c60 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1465,6 +1465,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;
@@ -1473,6 +1474,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;
@@ -1704,7 +1706,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);
Hi Haribabu Kommi
Thank you so much for the review and patch update. I should have done that
myself, but I've been really busy for the last few weeks. :(
Regards,
Marti
On Mon, Nov 16, 2015 at 4:46 AM, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:
Show quoted text
On Thu, Nov 5, 2015 at 10:20 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:On Tue, Sep 29, 2015 at 12:17 AM, Marti Raudsepp <marti@juffo.org>
wrote:
Hi list
The attached patch changes the behavior of multiple ALTER x SET SCHEMA
commands, to skip, rather than fail, when the old and new schema is
the same.The advantage is that it's now easier to write DDL scripts that are
indempotent.
This already matches the behavior of ALTER EXTENSION SET SCHEMA in
earlier versions, as well as many other SET-ish commands, e.g. ALTER
TABLE SET TABLESPACE, OWNER TO, CLUSTER ON, SET (storage_parameter...)
etc. I don't see why SET SCHEMA should be treated any differently.The code is written such that object_access_hook is still called for
each object.Regression tests included. I couldn't find any documentation that
needs changing.I went through the patch, following are my observations,
Patch applied with hunks and compiled with out warnings.
Basic tests are passed.In AlterTableNamespaceInternal function, if a table or matview called
for set schema,
If the object contains any constraints, the constraint gets updated
with new schema.In AlterTypeNamespaceInternal function, the InvokeObjectPostAlterHook
function
doesn't get called if the type is of composite type, domain and array
types as because
it just returns from top of the function.Most of the community members didn't find any problem in changing the
behavior, so here I attached updated patch with the above two corrections.Regards,
Hari Babu
Fujitsu Australia
On Mon, Nov 16, 2015 at 4:27 AM, Marti Raudsepp <marti@juffo.org> wrote:
Thank you so much for the review and patch update. I should have done that
myself, but I've been really busy for the last few weeks. :(
Maybe I'm having an attack of the stupids today, but it looks to me
like the changes to pg_constraint.c look awfully strange to me. In
the old code, if object_address_present() returns true, we continue,
skipping the rest of the loop. In the new code, we instead set
alreadyChanged to true. That causes both of the following if
statements, as revised, to fall out, so that we skip the rest of the
loop. Huh? Wouldn't a one line change to add oldNspId != newNspId to
the criteria for a simple_heap_update be just as good?
Backing up a bit, maybe we should be a bit more vigorous in treating a
same-namespace move as a no-op. That is, don't worry about calling
the post-alter hook in that case - just have AlterConstraintNamespaces
start by checking whether oldNspId == newNspid right at the top; if
so, return. The patch seems to have the idea that it is important to
call the post-alter hook even in that case, but I'm not sure whether
that's true. I'm not sure it's false, but I'm also not sure it's
true.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Nov 18, 2015 at 6:02 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Nov 16, 2015 at 4:27 AM, Marti Raudsepp <marti@juffo.org> wrote:
Thank you so much for the review and patch update. I should have done that
myself, but I've been really busy for the last few weeks. :(Maybe I'm having an attack of the stupids today, but it looks to me
like the changes to pg_constraint.c look awfully strange to me. In
the old code, if object_address_present() returns true, we continue,
skipping the rest of the loop. In the new code, we instead set
alreadyChanged to true. That causes both of the following if
statements, as revised, to fall out, so that we skip the rest of the
loop. Huh? Wouldn't a one line change to add oldNspId != newNspId to
the criteria for a simple_heap_update be just as good?
Yes, that's correct, the above change can be written as you suggested.
Updated patch attached with correction.
Backing up a bit, maybe we should be a bit more vigorous in treating a
same-namespace move as a no-op. That is, don't worry about calling
the post-alter hook in that case - just have AlterConstraintNamespaces
start by checking whether oldNspId == newNspid right at the top; if
so, return. The patch seems to have the idea that it is important to
call the post-alter hook even in that case, but I'm not sure whether
that's true. I'm not sure it's false, but I'm also not sure it's
true.
I am also not sure whether calling the post-alter hook in case of constraint is
necessarily required? but it was doing for other objects, so I suggested
that way.
Regards,
Hari Babu
Fujitsu Australia
Attachments:
0001-Skip-ALTER-x-SET-SCHEMA-if-the-schema-didn-t-change_v3.patchapplication/octet-stream; name=0001-Skip-ALTER-x-SET-SCHEMA-if-the-schema-didn-t-change_v3.patchDownload
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/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index 3c756f8..90e4409 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -725,8 +725,9 @@ AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
if (object_address_present(&thisobj, objsMoved))
continue;
-
- if (conform->connamespace == oldNspId)
+
+ /* Don't update if the object is already part of the namespace */
+ if (conform->connamespace == oldNspId && oldNspId != newNspId)
{
tup = heap_copytuple(tup);
conform = (Form_pg_constraint) GETSTRUCT(tup);
@@ -744,7 +745,6 @@ AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
}
InvokeObjectPostAlterHook(ConstraintRelationId, thisobj.objectId, 0);
-
add_exact_object_address(&thisobj, objsMoved);
}
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 44ea731..b1a8f75 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -11350,7 +11350,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);
@@ -11430,38 +11430,40 @@ AlterRelationNamespaceInternal(Relation classRel, Oid relOid,
thisobj.objectId = relOid;
thisobj.objectSubId = 0;
- /*
- * Do nothing when there's nothing to do.
- */
if (!object_address_present(&thisobj, objsMoved))
{
- /* check for duplicate name (more friendly than unique-index failure) */
- if (get_relname_relid(NameStr(classForm->relname),
- newNspOid) != InvalidOid)
- ereport(ERROR,
- (errcode(ERRCODE_DUPLICATE_TABLE),
- errmsg("relation \"%s\" already exists in schema \"%s\"",
- NameStr(classForm->relname),
- get_namespace_name(newNspOid))));
-
- /* classTup is a copy, so OK to scribble on */
- classForm->relnamespace = newNspOid;
-
- simple_heap_update(classRel, &classTup->t_self, classTup);
- CatalogUpdateIndexes(classRel, classTup);
-
- /* Update dependency on schema if caller said so */
- if (hasDependEntry &&
- changeDependencyFor(RelationRelationId,
- relOid,
- NamespaceRelationId,
- oldNspOid,
- newNspOid) != 1)
- elog(ERROR, "failed to change schema dependency for relation \"%s\"",
- NameStr(classForm->relname));
+ /*
+ * Do nothing when there's nothing to do.
+ */
+ if (oldNspOid != newNspOid)
+ {
+ /* check for duplicate name (more friendly than unique-index failure) */
+ if (get_relname_relid(NameStr(classForm->relname),
+ newNspOid) != InvalidOid)
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_TABLE),
+ errmsg("relation \"%s\" already exists in schema \"%s\"",
+ NameStr(classForm->relname),
+ get_namespace_name(newNspOid))));
+
+ /* classTup is a copy, so OK to scribble on */
+ classForm->relnamespace = newNspOid;
+
+ simple_heap_update(classRel, &classTup->t_self, classTup);
+ CatalogUpdateIndexes(classRel, classTup);
+
+ /* Update dependency on schema if caller said so */
+ if (hasDependEntry &&
+ changeDependencyFor(RelationRelationId,
+ relOid,
+ NamespaceRelationId,
+ oldNspOid,
+ newNspOid) != 1)
+ elog(ERROR, "failed to change schema dependency for relation \"%s\"",
+ NameStr(classForm->relname));
+ }
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..434bd8f 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -3520,18 +3520,22 @@ AlterTypeNamespaceInternal(Oid typeOid, Oid nspOid,
oldNspOid = typform->typnamespace;
arrayOid = typform->typarray;
- /* common checks on switching namespaces */
- CheckSetNamespace(oldNspOid, nspOid, TypeRelationId, typeOid);
+ /* The type is already there, nothing to do */
+ if (oldNspOid != nspOid)
+ {
+ /* common checks on switching namespaces */
+ CheckSetNamespace(oldNspOid, nspOid);
- /* check for duplicate name (more friendly than unique-index failure) */
- if (SearchSysCacheExists2(TYPENAMENSP,
- CStringGetDatum(NameStr(typform->typname)),
- ObjectIdGetDatum(nspOid)))
- ereport(ERROR,
- (errcode(ERRCODE_DUPLICATE_OBJECT),
- errmsg("type \"%s\" already exists in schema \"%s\"",
- NameStr(typform->typname),
- get_namespace_name(nspOid))));
+ /* check for duplicate name (more friendly than unique-index failure) */
+ if (SearchSysCacheExists2(TYPENAMENSP,
+ CStringGetDatum(NameStr(typform->typname)),
+ ObjectIdGetDatum(nspOid)))
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("type \"%s\" already exists in schema \"%s\"",
+ NameStr(typform->typname),
+ get_namespace_name(nspOid))));
+ }
/* Detect whether type is a composite type (but not a table rowtype) */
isCompositeType =
@@ -3547,13 +3551,16 @@ AlterTypeNamespaceInternal(Oid typeOid, Oid nspOid,
format_type_be(typeOid)),
errhint("Use ALTER TABLE instead.")));
- /* OK, modify the pg_type row */
+ if (oldNspOid != nspOid)
+ {
+ /* OK, modify the pg_type row */
- /* tup is a copy, so we can scribble directly on it */
- typform->typnamespace = nspOid;
+ /* tup is a copy, so we can scribble directly on it */
+ typform->typnamespace = nspOid;
- simple_heap_update(rel, &tup->t_self, tup);
- CatalogUpdateIndexes(rel, tup);
+ simple_heap_update(rel, &tup->t_self, tup);
+ CatalogUpdateIndexes(rel, tup);
+ }
/*
* Composite types have pg_class entries.
@@ -3592,8 +3599,8 @@ AlterTypeNamespaceInternal(Oid typeOid, Oid nspOid,
* Update dependency on schema, if any --- a table rowtype has not got
* one, and neither does an implicit array.
*/
- if ((isCompositeType || typform->typtype != TYPTYPE_COMPOSITE) &&
- !isImplicitArray)
+ if ((oldNspOid != nspOid) && ((isCompositeType || typform->typtype != TYPTYPE_COMPOSITE)
+ && !isImplicitArray))
if (changeDependencyFor(TypeRelationId, typeOid,
NamespaceRelationId, oldNspOid, nspOid) != 1)
elog(ERROR, "failed to change schema dependency for type %s",
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 50b3c45..d2b6f2c 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -2133,6 +2133,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;
@@ -2141,6 +2142,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;
@@ -2567,9 +2569,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 778791d..6740c60 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1465,6 +1465,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;
@@ -1473,6 +1474,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;
@@ -1704,7 +1706,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);
On Wed, Nov 18, 2015 at 12:32 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
On Wed, Nov 18, 2015 at 6:02 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Nov 16, 2015 at 4:27 AM, Marti Raudsepp <marti@juffo.org> wrote:
Thank you so much for the review and patch update. I should have done that
myself, but I've been really busy for the last few weeks. :(Maybe I'm having an attack of the stupids today, but it looks to me
like the changes to pg_constraint.c look awfully strange to me. In
the old code, if object_address_present() returns true, we continue,
skipping the rest of the loop. In the new code, we instead set
alreadyChanged to true. That causes both of the following if
statements, as revised, to fall out, so that we skip the rest of the
loop. Huh? Wouldn't a one line change to add oldNspId != newNspId to
the criteria for a simple_heap_update be just as good?Yes, that's correct, the above change can be written as you suggested.
Updated patch attached with correction.Backing up a bit, maybe we should be a bit more vigorous in treating a
same-namespace move as a no-op. That is, don't worry about calling
the post-alter hook in that case - just have AlterConstraintNamespaces
start by checking whether oldNspId == newNspid right at the top; if
so, return. The patch seems to have the idea that it is important to
call the post-alter hook even in that case, but I'm not sure whether
that's true. I'm not sure it's false, but I'm also not sure it's
true.I am also not sure whether calling the post-alter hook in case of constraint is
necessarily required? but it was doing for other objects, so I suggested
that way.
OK, committed with some additional cosmetic improvements.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers