Thread

1. recent ALTER whatever .. SET SCHEMA refactoring

   Robert Haas <robertmhaas@gmail.com> — 2012-12-20T14:24:46Z

   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
   
   
   

2. Re: recent ALTER whatever .. SET SCHEMA refactoring

   Kohei KaiGai <kaigai@kaigai.gr.jp> — 2013-01-02T15:48:42Z

   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>
   
   
   

3. Re: recent ALTER whatever .. SET SCHEMA refactoring

   Alvaro Herrera <alvherre@2ndquadrant.com> — 2013-01-07T19:14:29Z

   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
   
   
   

4. Re: recent ALTER whatever .. SET SCHEMA refactoring

   Robert Haas <robertmhaas@gmail.com> — 2013-01-07T20:47:13Z

   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
   
   
   

5. Re: recent ALTER whatever .. SET SCHEMA refactoring

   Kohei KaiGai <kaigai@kaigai.gr.jp> — 2013-01-08T09:05:19Z

   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>
   
   
   

6. Re: recent ALTER whatever .. SET SCHEMA refactoring

   Robert Haas <robertmhaas@gmail.com> — 2013-01-08T13:04:39Z

   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
   
   
   

7. Re: recent ALTER whatever .. SET SCHEMA refactoring

   Kohei KaiGai <kaigai@kaigai.gr.jp> — 2013-01-08T13:58:42Z

   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>
   

8. Re: recent ALTER whatever .. SET SCHEMA refactoring

   Alvaro Herrera <alvherre@2ndquadrant.com> — 2013-01-14T22:27:53Z

   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
   

9. Re: recent ALTER whatever .. SET SCHEMA refactoring

   Alvaro Herrera <alvherre@2ndquadrant.com> — 2013-01-15T14:21:05Z

   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
   
   
   

10. Re: recent ALTER whatever .. SET SCHEMA refactoring

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2013-01-15T14:48:47Z

    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>
    
    
    

11. Re: recent ALTER whatever .. SET SCHEMA refactoring

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2013-01-15T14:56:08Z

    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
    
    
    

12. Re: recent ALTER whatever .. SET SCHEMA refactoring

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2013-01-15T18:19:43Z

    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