Thread: 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
2012/12/20 Robert Haas <robertmhaas@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 <kaigai@kaigai.gr.jp>
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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Mon, Jan 7, 2013 at 2:14 PM, Alvaro Herrera <alvherre@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
2013/1/7 Robert Haas <robertmhaas@gmail.com>: > On Mon, Jan 7, 2013 at 2:14 PM, Alvaro Herrera <alvherre@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 <kaigai@kaigai.gr.jp>
On Tue, Jan 8, 2013 at 4:05 AM, Kohei KaiGai <kaigai@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
2013/1/8 Robert Haas <robertmhaas@gmail.com>:
> On Tue, Jan 8, 2013 at 4:05 AM, Kohei KaiGai <kaigai@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 <kaigai@kaigai.gr.jp>
Attachment
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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
2013/1/15 Alvaro Herrera <alvherre@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/CADyhKSWVqaA6iF5wVuW5EzLaiYyCYEE2zO9guqNKy8FRdLx5Gw@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 <kaigai@kaigai.gr.jp>
Kohei KaiGai escribió: > 2013/1/15 Alvaro Herrera <alvherre@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/CADyhKSWVqaA6iF5wVuW5EzLaiYyCYEE2zO9guqNKy8FRdLx5Gw@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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services