Re: [HACKERS] recent ALTER whatever .. SET SCHEMA refactoring
Alvaro Herrera escribió: Kohei KaiGai escribió: I'm probably saying same idea. It just adds invocation of external functions to check naming conflicts of functions or collation; that takes additional 4-lines for special case handling in AlterObjectNamespace_internal(). Okay, I can agree with this implementation plan. Actually, now that I look again, this is all completely broken, because the object already exists in schema foo message is using getObjectDescription infrastructure, which we agree to be completely wrong. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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
Re: [HACKERS] recent ALTER whatever .. SET SCHEMA refactoring
2013/1/15 Alvaro Herrera alvhe...@2ndquadrant.com: Alvaro Herrera escribió: Kohei KaiGai escribió: I'm probably saying same idea. It just adds invocation of external functions to check naming conflicts of functions or collation; that takes additional 4-lines for special case handling in AlterObjectNamespace_internal(). Okay, I can agree with this implementation plan. Actually, now that I look again, this is all completely broken, because the object already exists in schema foo message is using getObjectDescription infrastructure, which we agree to be completely wrong. http://www.postgresql.org/message-id/cadyhkswvqaa6if5wvuw5ezlaiyycyee2zo9guqnky8frdlx...@mail.gmail.com Does this patch help the trouble? It adds ereport_on_namespace_conflict() for error message generation instead of getObjectDescription() for ALTER RENAME primarily, but I also noticed it can be applied on getObjectDescription() of AlterObjectNamespace_internal. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] recent ALTER whatever .. SET SCHEMA refactoring
Kohei KaiGai escribió: 2013/1/15 Alvaro Herrera alvhe...@2ndquadrant.com: Alvaro Herrera escribió: Kohei KaiGai escribió: I'm probably saying same idea. It just adds invocation of external functions to check naming conflicts of functions or collation; that takes additional 4-lines for special case handling in AlterObjectNamespace_internal(). Okay, I can agree with this implementation plan. Actually, now that I look again, this is all completely broken, because the object already exists in schema foo message is using getObjectDescription infrastructure, which we agree to be completely wrong. http://www.postgresql.org/message-id/cadyhkswvqaa6if5wvuw5ezlaiyycyee2zo9guqnky8frdlx...@mail.gmail.com Does this patch help the trouble? It adds ereport_on_namespace_conflict() for error message generation instead of getObjectDescription() for ALTER RENAME primarily, but I also noticed it can be applied on getObjectDescription() of AlterObjectNamespace_internal. I was just going to look into that patch, thanks. Anyway I noticed that the getObjectDescriptionOids() in that path has been there since 9.1 introduced generic object support for SET SCHEMA in 55109313. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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
Re: [HACKERS] recent ALTER whatever .. SET SCHEMA refactoring
Robert Haas escribió: The recent SET SCHEMA refactoring has changed the error message that you get when trying to move a function into the schema that already contains it. I have committed 7ac5760fa2 which should fix this. Thanks for the report. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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
Re: [HACKERS] recent ALTER whatever .. SET SCHEMA refactoring
Kohei KaiGai escribió:
I'm probably saying same idea. It just adds invocation of external
functions to check naming conflicts of functions or collation; that
takes additional 4-lines for special case handling
in AlterObjectNamespace_internal().
Okay, I can agree with this implementation plan. I didn't like your
patch very much though; I'm not sure that the handling of collations was
sane. And the names of the AlterFooNamespace_oid() functions is now
completely misleading, because they no longer do that. So I renamed
them, and while at it I noticed that the collation case can share code
with the RENAME code path; the function case could probably do likewise,
but it becomes uglier.
Note that after this patch, the error message now cites the function
signature, not just the name. This is more consistent with other cases.
9.2 and HEAD:
alvherre=# alter function foo.f(int,int) set schema bar;
ERROR: function f already exists in schema bar
(this error is misleading, because there can validly be a f(int)
function in schema bar and that wouldn't cause a problem for this SET
SCHEMA command).
After my patch:
alvherre=# alter function foo.f(int,int) set schema bar;
ERROR: function f(integer, integer) already exists in schema bar
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
*** a/src/backend/commands/alter.c
--- b/src/backend/commands/alter.c
***
*** 19,27
--- 19,29
#include catalog/dependency.h
#include catalog/indexing.h
#include catalog/namespace.h
+ #include catalog/pg_collation.h
#include catalog/pg_largeobject.h
#include catalog/pg_largeobject_metadata.h
#include catalog/pg_namespace.h
+ #include catalog/pg_proc.h
#include commands/alter.h
#include commands/collationcmds.h
#include commands/conversioncmds.h
***
*** 146,185 ExecAlterObjectSchemaStmt(AlterObjectSchemaStmt *stmt)
{
switch (stmt-objectType)
{
- case OBJECT_AGGREGATE:
- return AlterFunctionNamespace(stmt-object, stmt-objarg, true,
- stmt-newschema);
-
- case OBJECT_COLLATION:
- return AlterCollationNamespace(stmt-object, stmt-newschema);
-
case OBJECT_EXTENSION:
return AlterExtensionNamespace(stmt-object, stmt-newschema);
! case OBJECT_FUNCTION:
! return AlterFunctionNamespace(stmt-object, stmt-objarg, false,
! stmt-newschema);
!
case OBJECT_SEQUENCE:
case OBJECT_TABLE:
case OBJECT_VIEW:
- case OBJECT_FOREIGN_TABLE:
return AlterTableNamespace(stmt);
- case OBJECT_TYPE:
case OBJECT_DOMAIN:
return AlterTypeNamespace(stmt-object, stmt-newschema,
stmt-objectType);
/* generic code path */
case OBJECT_CONVERSION:
case OBJECT_OPERATOR:
case OBJECT_OPCLASS:
case OBJECT_OPFAMILY:
! case OBJECT_TSPARSER:
case OBJECT_TSDICTIONARY:
case OBJECT_TSTEMPLATE:
- case OBJECT_TSCONFIGURATION:
{
Relation catalog;
Relation relation;
--- 148,179
{
switch (stmt-objectType)
{
case OBJECT_EXTENSION:
return AlterExtensionNamespace(stmt-object, stmt-newschema);
! case OBJECT_FOREIGN_TABLE:
case OBJECT_SEQUENCE:
case OBJECT_TABLE:
case OBJECT_VIEW:
return AlterTableNamespace(stmt);
case OBJECT_DOMAIN:
+ case OBJECT_TYPE:
return AlterTypeNamespace(stmt-object, stmt-newschema,
stmt-objectType);
/* generic code path */
+ case OBJECT_AGGREGATE:
+ case OBJECT_COLLATION:
case OBJECT_CONVERSION:
+ case OBJECT_FUNCTION:
case OBJECT_OPERATOR:
case OBJECT_OPCLASS:
case OBJECT_OPFAMILY:
! case OBJECT_TSCONFIGURATION:
case OBJECT_TSDICTIONARY:
+ case OBJECT_TSPARSER:
case OBJECT_TSTEMPLATE:
{
Relation catalog;
Relation relation;
***
*** 253,270 AlterObjectNamespace_oid(Oid classId, Oid objid, Oid nspOid,
break;
}
- case OCLASS_PROC:
- oldNspOid = AlterFunctionNamespace_oid(objid, nspOid);
- break;
-
case OCLASS_TYPE:
oldNspOid = AlterTypeNamespace_oid(objid, nspOid, objsMoved);
break;
case OCLASS_COLLATION:
- oldNspOid = AlterCollationNamespace_oid(objid, nspOid);
- break;
-
case OCLASS_CONVERSION:
case OCLASS_OPERATOR:
case OCLASS_OPCLASS:
--- 247,258
break;
}
case OCLASS_TYPE:
oldNspOid = AlterTypeNamespace_oid(objid, nspOid, objsMoved);
break;
+ case OCLASS_PROC:
case OCLASS_COLLATION:
case OCLASS_CONVERSION:
case OCLASS_OPERATOR:
case OCLASS_OPCLASS:
***
*** 380,385 AlterObjectNamespace_internal(Relation rel, Oid objid, Oid nspOid)
--- 368,385
errmsg(%s already exists in schema \%s\,
getObjectDescriptionOids(classId, objid),
get_namespace_name(nspOid;
+ else if (classId == ProcedureRelationId)
+ IsThereFunctionInNamespace(objid, nspOid);
+ else if (classId ==
Re: [HACKERS] recent ALTER whatever .. SET SCHEMA refactoring
2013/1/7 Robert Haas robertmh...@gmail.com: On Mon, Jan 7, 2013 at 2:14 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Kohei KaiGai escribió: Function and collation are candidates of this special case handling; here are just two kinds of object. Another idea is to add a function-pointer as argument of AlterNamespace_internal for (upcoming) object classes that takes special handling for detection of name collision. My personal preference is the later one, rather than hardwired special case handling. However, it may be too elaborate to handle just two exceptions. I think this idea is fine. Pass a function pointer which is only not-NULL for the two exceptional cases; the code should have an Assert that either the function pointer is passed, or there is a nameCacheId to use. That way, the object types we already handle in the simpler way do not get any more complicated than they are today, and we're not forced to create useless callbacks for objects were the lookup is trivial. The function pointer should return boolean, true when the function/collation is already in the given schema; that way, the message wording is only present in AlterObjectNamespace_internal. It seems overly complex to me. What's wrong with putting special-case logic directly into the function? That seems cleaner and easier to understand, and there's no real downside AFAICS. We have similar special cases elsewhere; the code can't be simpler than the actual logic. Does it make sense an idea to invoke AlterFunctionNamespace_oid() or AlterCollationNamespace_oid() from AlterObjectNamespace_internal() for checks of namespace conflicts? It can handle special cases with keeping modularity between common and specific parts. Let's consider function pointer when we have mode than 5 object classes that needs special treatment. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] recent ALTER whatever .. SET SCHEMA refactoring
On Tue, Jan 8, 2013 at 4:05 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: Does it make sense an idea to invoke AlterFunctionNamespace_oid() or AlterCollationNamespace_oid() from AlterObjectNamespace_internal() for checks of namespace conflicts? It can handle special cases with keeping modularity between common and specific parts. Let's consider function pointer when we have mode than 5 object classes that needs special treatment. Unless I'm gravely mistaken, we're only talking about a handful of lines of code. We have lots of functions, in objectaddress.c for example, whose behavior is conditional on the type of object that they are operating on. And we just write out all the cases. I'm not understanding why we need to take a substantially more complex approach here. -- 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
Re: [HACKERS] recent ALTER whatever .. SET SCHEMA refactoring
2013/1/8 Robert Haas robertmh...@gmail.com: On Tue, Jan 8, 2013 at 4:05 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: Does it make sense an idea to invoke AlterFunctionNamespace_oid() or AlterCollationNamespace_oid() from AlterObjectNamespace_internal() for checks of namespace conflicts? It can handle special cases with keeping modularity between common and specific parts. Let's consider function pointer when we have mode than 5 object classes that needs special treatment. Unless I'm gravely mistaken, we're only talking about a handful of lines of code. We have lots of functions, in objectaddress.c for example, whose behavior is conditional on the type of object that they are operating on. And we just write out all the cases. I'm not understanding why we need to take a substantially more complex approach here. I'm probably saying same idea. It just adds invocation of external functions to check naming conflicts of functions or collation; that takes additional 4-lines for special case handling in AlterObjectNamespace_internal(). Do you have different image for the special case handling? @@ -380,6 +368,10 @@ AlterObjectNamespace_internal(Relation rel, Oid objid, Oid nspOid) errmsg(%s already exists in schema \%s\, getObjectDescriptionOids(classId, objid), get_namespace_name(nspOid; + else if (classId == ProcedureRelationId) + AlterFunctionNamespace_oid(rel, objid, nspOid); + else if (classId == CollationRelationId) + AlterCollationNamespace_oid(rel, objid, nspOid); /* Build modified tuple */ values = palloc0(RelationGetNumberOfAttributes(rel) * sizeof(Datum)); Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp pgsql-v9.3-alter-namespace-specials.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] recent ALTER whatever .. SET SCHEMA refactoring
Kohei KaiGai escribió: Function and collation are candidates of this special case handling; here are just two kinds of object. Another idea is to add a function-pointer as argument of AlterNamespace_internal for (upcoming) object classes that takes special handling for detection of name collision. My personal preference is the later one, rather than hardwired special case handling. However, it may be too elaborate to handle just two exceptions. I think this idea is fine. Pass a function pointer which is only not-NULL for the two exceptional cases; the code should have an Assert that either the function pointer is passed, or there is a nameCacheId to use. That way, the object types we already handle in the simpler way do not get any more complicated than they are today, and we're not forced to create useless callbacks for objects were the lookup is trivial. The function pointer should return boolean, true when the function/collation is already in the given schema; that way, the message wording is only present in AlterObjectNamespace_internal. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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
Re: [HACKERS] recent ALTER whatever .. SET SCHEMA refactoring
On Mon, Jan 7, 2013 at 2:14 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Kohei KaiGai escribió: Function and collation are candidates of this special case handling; here are just two kinds of object. Another idea is to add a function-pointer as argument of AlterNamespace_internal for (upcoming) object classes that takes special handling for detection of name collision. My personal preference is the later one, rather than hardwired special case handling. However, it may be too elaborate to handle just two exceptions. I think this idea is fine. Pass a function pointer which is only not-NULL for the two exceptional cases; the code should have an Assert that either the function pointer is passed, or there is a nameCacheId to use. That way, the object types we already handle in the simpler way do not get any more complicated than they are today, and we're not forced to create useless callbacks for objects were the lookup is trivial. The function pointer should return boolean, true when the function/collation is already in the given schema; that way, the message wording is only present in AlterObjectNamespace_internal. It seems overly complex to me. What's wrong with putting special-case logic directly into the function? That seems cleaner and easier to understand, and there's no real downside AFAICS. We have similar special cases elsewhere; the code can't be simpler than the actual logic. -- 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
Re: [HACKERS] recent ALTER whatever .. SET SCHEMA refactoring
2012/12/20 Robert Haas robertmh...@gmail.com: The recent SET SCHEMA refactoring has changed the error message that you get when trying to move a function into the schema that already contains it. For a table, as ever, you get: rhaas=# create table foo (a int); CREATE TABLE rhaas=# alter table foo set schema public; ERROR: table foo is already in schema public Functions used to produce the same message, but not any more: rhaas=# create function foo(a int) returns int as $$select 1$$ language sql; CREATE FUNCTION rhaas=# alter function foo(int) set schema public; ERROR: function foo already exists in schema public The difference is that the first error message is complaining about an operation that is a no-op, whereas the second one is complaining about a name collision. It seems a bit off in this case because the name collision is between the function and itself, not the function and some other object with a conflicting name. The root of the problem is that AlterObjectNamespace_internal generates both error messages and does the checks in the correct order, but for functions, AlterFunctionNamespace_oid has a second copy of the conflicting-names check that is argument-type aware, which happens before the same-schema check in AlterObjectNamespace_internal. IMHO, we ought to fix this by getting rid of the check in AlterFunctionNamespace_oid and adding an appropriate special case for functions in AlterObjectNamespace_internal that knows how to do the check correctly. It's not a huge deal, but it seems confusing to have AlterObjectNamespace_internal know how to do the check correctly in some cases but not others. Sorry for my oversight this message. It seems to me it is a reasonable solution to have a special case for object types that does not support simple name looking-up with name itself or combination of name + namespace. Function and collation are candidates of this special case handling; here are just two kinds of object. Another idea is to add a function-pointer as argument of AlterNamespace_internal for (upcoming) object classes that takes special handling for detection of name collision. My personal preference is the later one, rather than hardwired special case handling. However, it may be too elaborate to handle just two exceptions. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] recent ALTER whatever .. SET SCHEMA refactoring
The recent SET SCHEMA refactoring has changed the error message that you get when trying to move a function into the schema that already contains it. For a table, as ever, you get: rhaas=# create table foo (a int); CREATE TABLE rhaas=# alter table foo set schema public; ERROR: table foo is already in schema public Functions used to produce the same message, but not any more: rhaas=# create function foo(a int) returns int as $$select 1$$ language sql; CREATE FUNCTION rhaas=# alter function foo(int) set schema public; ERROR: function foo already exists in schema public The difference is that the first error message is complaining about an operation that is a no-op, whereas the second one is complaining about a name collision. It seems a bit off in this case because the name collision is between the function and itself, not the function and some other object with a conflicting name. The root of the problem is that AlterObjectNamespace_internal generates both error messages and does the checks in the correct order, but for functions, AlterFunctionNamespace_oid has a second copy of the conflicting-names check that is argument-type aware, which happens before the same-schema check in AlterObjectNamespace_internal. IMHO, we ought to fix this by getting rid of the check in AlterFunctionNamespace_oid and adding an appropriate special case for functions in AlterObjectNamespace_internal that knows how to do the check correctly. It's not a huge deal, but it seems confusing to have AlterObjectNamespace_internal know how to do the check correctly in some cases but not others. -- 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
