Thread

1. ALTER command reworks

   Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-07-24T05:24:11Z

   The attached patch is my homework at the last commit fest of v9.2.
   
   It consolidates some similar routines into a single generic routines
   that handles ALTER RENAME TO, OWNER TO and SET SCHEMA
   according to the ObjectProperty in objectaddress.c, but here is
   no functionality enhancement.
   
   As existing AlterObjectNamespace() doing, this generic routine
   handles only case when a catalog entry is updated without any
   extra permission checks or other stuffs.
   Thus, RenameSchema was not consolidated due to CREATE
   permission checks towards the database, for example.
   
   As patched chunk in commands/alter.c shows, 13 of special
   routines were consolidated into AlterObjectRename, 16 of
   special ones were consolidated into AlterObjectNamespace,
   and 20 of special ones were consolidated into AlterObjectOwner().
   
   My motivation is for minimization of number of places to put
   object-access-hook around alter statement. Probably, it shall
   be located around or just after simple_heap_update().
   This is a groundwork towards ALTER command support in
   sepgsql module.
   
   Thanks,
   -- 
   KaiGai Kohei <kaigai@kaigai.gr.jp>
   

2. Re: ALTER command reworks

   Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-08-13T19:35:58Z

   The attached patch is a refreshed version of ALTER command
   reworks towards the latest tree. Here is few big changes except
   for code integration of the code to rename event triggers.
   
   BTW, I had to adjust between oid of pg_largeobject_metadata
   and pg_largeobject on three points of this patch, like:
   
           if (classId == LargeObjectRelationId)
               classId = LargeObjectMetadataRelationId;
   
   When we supported largeobject permission features, we put
   special handling to track dependency of its ownership.
   The pg_depend records oid of pg_largeobject, instead of
   pg_largeobject_metadata. Thus, we cannot use classId of
   ObjectAddress being returned from get_object_address()
   as an argument of heap_open() as is, if it indicates oid of
   pg_largeobject.
   
   Was it a right decision to track dependency of large object using
   oid of pg_largeobject, instead of pg_largeobject_metadata?
   IIRC, the reason why we used oid of pg_largeobject is backward
   compatibility for applications that tries to reference pg_depend
   with built-in oids.
   
   Don't we need to change the specification?
   Or, do we continue to implement the code carefully?
   
   Thanks,
   
   2012/7/24 Kohei KaiGai <kaigai@kaigai.gr.jp>:
   > The attached patch is my homework at the last commit fest of v9.2.
   >
   > It consolidates some similar routines into a single generic routines
   > that handles ALTER RENAME TO, OWNER TO and SET SCHEMA
   > according to the ObjectProperty in objectaddress.c, but here is
   > no functionality enhancement.
   >
   > As existing AlterObjectNamespace() doing, this generic routine
   > handles only case when a catalog entry is updated without any
   > extra permission checks or other stuffs.
   > Thus, RenameSchema was not consolidated due to CREATE
   > permission checks towards the database, for example.
   >
   > As patched chunk in commands/alter.c shows, 13 of special
   > routines were consolidated into AlterObjectRename, 16 of
   > special ones were consolidated into AlterObjectNamespace,
   > and 20 of special ones were consolidated into AlterObjectOwner().
   >
   > My motivation is for minimization of number of places to put
   > object-access-hook around alter statement. Probably, it shall
   > be located around or just after simple_heap_update().
   > This is a groundwork towards ALTER command support in
   > sepgsql module.
   >
   > Thanks,
   > --
   > KaiGai Kohei <kaigai@kaigai.gr.jp>
   
   
   
   -- 
   KaiGai Kohei <kaigai@kaigai.gr.jp>
   

3. Re: ALTER command reworks

   Robert Haas <robertmhaas@gmail.com> — 2012-08-30T19:24:06Z

   On Mon, Aug 13, 2012 at 3:35 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
   > The attached patch is a refreshed version of ALTER command
   > reworks towards the latest tree. Here is few big changes except
   > for code integration of the code to rename event triggers.
   
   This seems to have bit-rotted a bit.  Please rebase.
   
   > BTW, I had to adjust between oid of pg_largeobject_metadata
   > and pg_largeobject on three points of this patch, like:
   >
   >         if (classId == LargeObjectRelationId)
   >             classId = LargeObjectMetadataRelationId;
   >
   > When we supported largeobject permission features, we put
   > special handling to track dependency of its ownership.
   > The pg_depend records oid of pg_largeobject, instead of
   > pg_largeobject_metadata. Thus, we cannot use classId of
   > ObjectAddress being returned from get_object_address()
   > as an argument of heap_open() as is, if it indicates oid of
   > pg_largeobject.
   >
   > Was it a right decision to track dependency of large object using
   > oid of pg_largeobject, instead of pg_largeobject_metadata?
   > IIRC, the reason why we used oid of pg_largeobject is backward
   > compatibility for applications that tries to reference pg_depend
   > with built-in oids.
   
   I think it was a terrible decision and I'm pretty sure I said as much
   at the time, but I lost the argument, so I'm inclined to think we're
   stuck with continuing to support that kludge.
   
   Some other preliminary comments:
   
   - Surely you need to take AccessExclusiveLock on the object being
   renamed, not just ShareUpdateExclusiveLock.
   - I don't think it's acceptable to assemble the object-type
   "object-name" already exists message using getObjectDescription();
   it's not good for translators.  Use an array of messages, one per
   object-type, as we have done in other cases.
   - I would like to handle either the RENAME case or the ALTER OWNER
   case in one patch, and the other in a follow-on patch.  Can you pick
   one to do first and remove everything related to the other one?
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   
   
   

4. Re: ALTER command reworks

   Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-08-31T04:27:34Z

   2012/8/30 Robert Haas <robertmhaas@gmail.com>:
   > On Mon, Aug 13, 2012 at 3:35 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
   >> The attached patch is a refreshed version of ALTER command
   >> reworks towards the latest tree. Here is few big changes except
   >> for code integration of the code to rename event triggers.
   >
   > This seems to have bit-rotted a bit.  Please rebase.
   >
   >> BTW, I had to adjust between oid of pg_largeobject_metadata
   >> and pg_largeobject on three points of this patch, like:
   >>
   >>         if (classId == LargeObjectRelationId)
   >>             classId = LargeObjectMetadataRelationId;
   >>
   >> When we supported largeobject permission features, we put
   >> special handling to track dependency of its ownership.
   >> The pg_depend records oid of pg_largeobject, instead of
   >> pg_largeobject_metadata. Thus, we cannot use classId of
   >> ObjectAddress being returned from get_object_address()
   >> as an argument of heap_open() as is, if it indicates oid of
   >> pg_largeobject.
   >>
   >> Was it a right decision to track dependency of large object using
   >> oid of pg_largeobject, instead of pg_largeobject_metadata?
   >> IIRC, the reason why we used oid of pg_largeobject is backward
   >> compatibility for applications that tries to reference pg_depend
   >> with built-in oids.
   >
   > I think it was a terrible decision and I'm pretty sure I said as much
   > at the time, but I lost the argument, so I'm inclined to think we're
   > stuck with continuing to support that kludge.
   >
   OK, we will keep to implement carefully...
   
   > Some other preliminary comments:
   >
   > - Surely you need to take AccessExclusiveLock on the object being
   > renamed, not just ShareUpdateExclusiveLock.
   > - I don't think it's acceptable to assemble the object-type
   > "object-name" already exists message using getObjectDescription();
   > it's not good for translators.  Use an array of messages, one per
   > object-type, as we have done in other cases.
   > - I would like to handle either the RENAME case or the ALTER OWNER
   > case in one patch, and the other in a follow-on patch.  Can you pick
   > one to do first and remove everything related to the other one?
   >
   OK, I'll split the patch into three (isn't it?) portions; RENAME, SET OWNER
   and SET SCHEMA, with all above your suggestions.
   
   Thanks,
   -- 
   KaiGai Kohei <kaigai@kaigai.gr.jp>
   
   
   

5. Re: ALTER command reworks

   Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-09-10T11:08:32Z

   2012/8/31 Kohei KaiGai <kaigai@kaigai.gr.jp>:
   > 2012/8/30 Robert Haas <robertmhaas@gmail.com>:
   >> On Mon, Aug 13, 2012 at 3:35 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
   >>> The attached patch is a refreshed version of ALTER command
   >>> reworks towards the latest tree. Here is few big changes except
   >>> for code integration of the code to rename event triggers.
   >>
   >> This seems to have bit-rotted a bit.  Please rebase.
   >>
   >>> BTW, I had to adjust between oid of pg_largeobject_metadata
   >>> and pg_largeobject on three points of this patch, like:
   >>>
   >>>         if (classId == LargeObjectRelationId)
   >>>             classId = LargeObjectMetadataRelationId;
   >>>
   >>> When we supported largeobject permission features, we put
   >>> special handling to track dependency of its ownership.
   >>> The pg_depend records oid of pg_largeobject, instead of
   >>> pg_largeobject_metadata. Thus, we cannot use classId of
   >>> ObjectAddress being returned from get_object_address()
   >>> as an argument of heap_open() as is, if it indicates oid of
   >>> pg_largeobject.
   >>>
   >>> Was it a right decision to track dependency of large object using
   >>> oid of pg_largeobject, instead of pg_largeobject_metadata?
   >>> IIRC, the reason why we used oid of pg_largeobject is backward
   >>> compatibility for applications that tries to reference pg_depend
   >>> with built-in oids.
   >>
   >> I think it was a terrible decision and I'm pretty sure I said as much
   >> at the time, but I lost the argument, so I'm inclined to think we're
   >> stuck with continuing to support that kludge.
   >>
   > OK, we will keep to implement carefully...
   >
   >> Some other preliminary comments:
   >>
   >> - Surely you need to take AccessExclusiveLock on the object being
   >> renamed, not just ShareUpdateExclusiveLock.
   >> - I don't think it's acceptable to assemble the object-type
   >> "object-name" already exists message using getObjectDescription();
   >> it's not good for translators.  Use an array of messages, one per
   >> object-type, as we have done in other cases.
   >> - I would like to handle either the RENAME case or the ALTER OWNER
   >> case in one patch, and the other in a follow-on patch.  Can you pick
   >> one to do first and remove everything related to the other one?
   >>
   > OK, I'll split the patch into three (isn't it?) portions; RENAME, SET OWNER
   > and SET SCHEMA, with all above your suggestions.
   >
   As attached, I split off the original one into three portions; for set-schema,
   set-owner and rename-to. Please apply them in order of patch filename.
   Regarding to the error message, RenameErrorMsgAlreadyExists() was added
   to handle per object type messages in case when aclcheck_error() is not
   available to utilize.
   All the regression test is contained with the 1st patch to make sure the
   series of reworks does not change existing behaviors.
   
   Thanks,
   -- 
   KaiGai Kohei <kaigai@kaigai.gr.jp>
   

6. Re: ALTER command reworks

   Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-09-27T20:38:04Z

   Excerpts from Kohei KaiGai's message of lun sep 10 08:08:32 -0300 2012:
   
   > As attached, I split off the original one into three portions; for set-schema,
   > set-owner and rename-to. Please apply them in order of patch filename.
   > Regarding to the error message, RenameErrorMsgAlreadyExists() was added
   > to handle per object type messages in case when aclcheck_error() is not
   > available to utilize.
   > All the regression test is contained with the 1st patch to make sure the
   > series of reworks does not change existing behaviors.
   
   I checked this and noticed that the regression test has a couple of
   failures -- see below.  I think we should remove the test for the first
   two hunks, and fix the query for the final one to remove the offending
   column.
   
   The good news is that I ran this without applying the whole patch, only
   the new regression test' files.  I didn't check the files in detail.
   
   I have skimmed over the patches and they seem reasonable too; I will
   look at them in more detail later.  I think we should go ahead and apply
   the (tweaked) regression test alone, as a first step.
   
   *** /pgsql/source/HEAD/src/test/regress/expected/alter_generic.out	2012-09-27 17:23:05.000000000 -0300
   --- /home/alvherre/Code/pgsql/build/HEAD/src/test/regress/results/alter_generic.out	2012-09-27 17:29:21.000000000 -0300
   ***************
   *** 106,112 ****
     CREATE COLLATION alt_coll1 (locale = 'C');
     CREATE COLLATION alt_coll2 (locale = 'C');
     ALTER COLLATION alt_coll1 RENAME TO alt_coll2;  -- failed (name conflict)
   ! ERROR:  collation "alt_coll2" for encoding "SQL_ASCII" already exists in schema "alt_nsp1"
     ALTER COLLATION alt_coll1 RENAME TO alt_coll3;  -- OK
     ALTER COLLATION alt_coll2 OWNER TO regtest_alter_user2;  -- failed (no role membership)
     ERROR:  must be member of role "regtest_alter_user2"
   --- 106,112 ----
     CREATE COLLATION alt_coll1 (locale = 'C');
     CREATE COLLATION alt_coll2 (locale = 'C');
     ALTER COLLATION alt_coll1 RENAME TO alt_coll2;  -- failed (name conflict)
   ! ERROR:  collation "alt_coll2" for encoding "UTF8" already exists in schema "alt_nsp1"
     ALTER COLLATION alt_coll1 RENAME TO alt_coll3;  -- OK
     ALTER COLLATION alt_coll2 OWNER TO regtest_alter_user2;  -- failed (no role membership)
     ERROR:  must be member of role "regtest_alter_user2"
   ***************
   *** 125,131 ****
     ALTER COLLATION alt_coll3 SET SCHEMA alt_nsp2;  -- failed (not owner)
     ERROR:  must be owner of collation alt_coll3
     ALTER COLLATION alt_coll2 SET SCHEMA alt_nsp2;  -- failed (name conflict)
   ! ERROR:  collation "alt_coll2" for encoding "SQL_ASCII" already exists in schema "alt_nsp2"
     RESET SESSION AUTHORIZATION;
     SELECT n.nspname, c.collname, a.rolname
       FROM pg_collation c, pg_namespace n, pg_authid a
   --- 125,131 ----
     ALTER COLLATION alt_coll3 SET SCHEMA alt_nsp2;  -- failed (not owner)
     ERROR:  must be owner of collation alt_coll3
     ALTER COLLATION alt_coll2 SET SCHEMA alt_nsp2;  -- failed (name conflict)
   ! ERROR:  collation "alt_coll2" for encoding "UTF8" already exists in schema "alt_nsp2"
     RESET SESSION AUTHORIZATION;
     SELECT n.nspname, c.collname, a.rolname
       FROM pg_collation c, pg_namespace n, pg_authid a
   ***************
   *** 334,341 ****
       ORDER BY nspname, opfname;
      nspname  | opfname  | amname |       rolname       
     ----------+----------+--------+---------------------
   !  alt_nsp1 | alt_opc1 | hash   | kaigai
   !  alt_nsp1 | alt_opc2 | hash   | kaigai
      alt_nsp1 | alt_opf2 | hash   | regtest_alter_user2
      alt_nsp1 | alt_opf3 | hash   | regtest_alter_user1
      alt_nsp1 | alt_opf4 | hash   | regtest_alter_user2
   --- 334,341 ----
       ORDER BY nspname, opfname;
      nspname  | opfname  | amname |       rolname       
     ----------+----------+--------+---------------------
   !  alt_nsp1 | alt_opc1 | hash   | alvherre
   !  alt_nsp1 | alt_opc2 | hash   | alvherre
      alt_nsp1 | alt_opf2 | hash   | regtest_alter_user2
      alt_nsp1 | alt_opf3 | hash   | regtest_alter_user1
      alt_nsp1 | alt_opf4 | hash   | regtest_alter_user2
   
   ======================================================================
   
   -- 
   Álvaro Herrera                http://www.2ndQuadrant.com/
   PostgreSQL Development, 24x7 Support, Training & Services
   
   
   

7. Re: ALTER command reworks

   Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-09-28T21:17:32Z

   Excerpts from Kohei KaiGai's message of lun sep 10 08:08:32 -0300 2012:
   
   > As attached, I split off the original one into three portions; for set-schema,
   > set-owner and rename-to. Please apply them in order of patch filename.
   
   Hmm, in the first patch, it seems to me we can simplify
   AlterObjectNamespace's signature: instead of passing all the details of
   the object class (cache Ids and attribute numbers and so on), just do
   
   AlterObjectNamespace(catalog-containing-object, objectId, newNamespaceOid)
   
   AlterObjectNamespace then looks up the catcache_oid and so on
   internally.  The only difference from what's happening in the submitted
   patch is that in the AlterCollationNamespace case, AlterObjectNamespace
   would have to look them up instead of getting them directly from the
   caller as the patch currently has it.
   
   -- 
   Álvaro Herrera                http://www.2ndQuadrant.com/
   PostgreSQL Development, 24x7 Support, Training & Services
   
   
   

8. Re: ALTER command reworks

   Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-10-02T14:19:17Z

   Excerpts from Alvaro Herrera's message of vie sep 28 18:17:32 -0300 2012:
   > Excerpts from Kohei KaiGai's message of lun sep 10 08:08:32 -0300 2012:
   > 
   > > As attached, I split off the original one into three portions; for set-schema,
   > > set-owner and rename-to. Please apply them in order of patch filename.
   > 
   > Hmm, in the first patch, it seems to me we can simplify
   > AlterObjectNamespace's signature: instead of passing all the details of
   > the object class (cache Ids and attribute numbers and so on), just do
   > 
   > AlterObjectNamespace(catalog-containing-object, objectId, newNamespaceOid)
   
   Like in the attached reworked version, in which I renamed the function
   to AlterObjectNamespace_internal because the other name seemed a bit
   wrong in the fact of the existing AlterObjectNamespace_oid.
   
   I also made the ALTER FUNCTION case go through
   AlterObjectNamespace_internal; it seems pointless to have a separate
   code path to go through when the generic one does just fine (also, this
   makes functions identical to collations in implementation).  That's one
   less hook point for sepgsql, I guess.
   
   -- 
   Álvaro Herrera                http://www.2ndQuadrant.com/
   PostgreSQL Development, 24x7 Support, Training & Services
   

9. Re: ALTER command reworks

   Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-10-03T21:25:54Z

   Excerpts from Kohei KaiGai's message of lun sep 10 08:08:32 -0300 2012:
   
   > As attached, I split off the original one into three portions; for set-schema,
   > set-owner and rename-to. Please apply them in order of patch filename.
   > Regarding to the error message, RenameErrorMsgAlreadyExists() was added
   > to handle per object type messages in case when aclcheck_error() is not
   > available to utilize.
   
   I have pushed the regression tests and parts 1 and 2.  Only part 3 is
   missing from this series, but I'm not as sure about that one as the
   other two.  Not really a fan of RenameErrorMsgAlreadyExists() as coded,
   but I'll think more about it.
   
   -- 
   Álvaro Herrera                http://www.2ndQuadrant.com/
   PostgreSQL Development, 24x7 Support, Training & Services
   
   
   

10. Re: ALTER command reworks

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-10-03T21:28:00Z

    Excerpts from Alvaro Herrera's message of mié oct 03 18:25:54 -0300 2012:
    > Excerpts from Kohei KaiGai's message of lun sep 10 08:08:32 -0300 2012:
    > 
    > > As attached, I split off the original one into three portions; for set-schema,
    > > set-owner and rename-to. Please apply them in order of patch filename.
    > > Regarding to the error message, RenameErrorMsgAlreadyExists() was added
    > > to handle per object type messages in case when aclcheck_error() is not
    > > available to utilize.
    > 
    > I have pushed the regression tests and parts 1 and 2.  Only part 3 is
    > missing from this series, but I'm not as sure about that one as the
    > other two.  Not really a fan of RenameErrorMsgAlreadyExists() as coded,
    > but I'll think more about it.
    
    I forgot to mention: I think with a little more effort (a callback to be
    registered as the permission check to run during SET OWNER, maybe?) we
    could move the foreign stuff and event triggers into the generic SET
    OWNER implementation.
    
    -- 
    Álvaro Herrera                http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

11. Re: ALTER command reworks

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-10-05T14:50:43Z

    Kohei KaiGai escribió:
    > 2012/8/31 Kohei KaiGai <kaigai@kaigai.gr.jp>:
    > > 2012/8/30 Robert Haas <robertmhaas@gmail.com>:
    > >> On Mon, Aug 13, 2012 at 3:35 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    
    > >>> Was it a right decision to track dependency of large object using
    > >>> oid of pg_largeobject, instead of pg_largeobject_metadata?
    > >>> IIRC, the reason why we used oid of pg_largeobject is backward
    > >>> compatibility for applications that tries to reference pg_depend
    > >>> with built-in oids.
    > >>
    > >> I think it was a terrible decision and I'm pretty sure I said as much
    > >> at the time, but I lost the argument, so I'm inclined to think we're
    > >> stuck with continuing to support that kludge.
    > >>
    > > OK, we will keep to implement carefully...
    
    After reviewing this patch, I think we need to revisit this decision.
    
    > > OK, I'll split the patch into three (isn't it?) portions; RENAME, SET OWNER
    > > and SET SCHEMA, with all above your suggestions.
    > >
    > As attached, I split off the original one into three portions; for set-schema,
    > set-owner and rename-to. Please apply them in order of patch filename.
    > Regarding to the error message, RenameErrorMsgAlreadyExists() was added
    > to handle per object type messages in case when aclcheck_error() is not
    > available to utilize.
    
    Here's the remaining piece; the RENAME part.  I have reworked it a bit,
    but it needs a bit more work yet.  Note this:
    
    alvherre=# alter function foo.g(int, int) rename to g;
    ERROR:  function foo.g(integer,integer) already exists in schema "foo"
    
    The previous code would have said
    
    alvherre=# alter function foo.g(int, int) rename to g;
    ERROR:  function g(integer, integer) already exists in schema "foo"
    
    Note that the function name is not qualified here.  The difference is
    that the original code was calling funcname_signature_string() to format
    the function name, and the new code is calling format_procedure().  This
    seems wrong to me; please rework.
    
    I haven't checked other object renames, but I think they should be okay
    because functions are the only objects for which we pass the OID instead
    of the name to the error message function.
    
    -- 
    Álvaro Herrera                http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
    

12. Re: ALTER command reworks

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-10-06T18:58:24Z

    2012/10/2 Alvaro Herrera <alvherre@2ndquadrant.com>:
    > Excerpts from Alvaro Herrera's message of vie sep 28 18:17:32 -0300 2012:
    >> Excerpts from Kohei KaiGai's message of lun sep 10 08:08:32 -0300 2012:
    >>
    >> > As attached, I split off the original one into three portions; for set-schema,
    >> > set-owner and rename-to. Please apply them in order of patch filename.
    >>
    >> Hmm, in the first patch, it seems to me we can simplify
    >> AlterObjectNamespace's signature: instead of passing all the details of
    >> the object class (cache Ids and attribute numbers and so on), just do
    >>
    >> AlterObjectNamespace(catalog-containing-object, objectId, newNamespaceOid)
    >
    > Like in the attached reworked version, in which I renamed the function
    > to AlterObjectNamespace_internal because the other name seemed a bit
    > wrong in the fact of the existing AlterObjectNamespace_oid.
    >
    > I also made the ALTER FUNCTION case go through
    > AlterObjectNamespace_internal; it seems pointless to have a separate
    > code path to go through when the generic one does just fine (also, this
    > makes functions identical to collations in implementation).  That's one
    > less hook point for sepgsql, I guess.
    >
    Thanks for your reviewing, and sorry for my late response.
    
    I definitely agree with your solution. The reason why my original patch
    had separate code path for function and collation was they took
    additional elements (such as argument-list of function) to check
    duplicate names. So, I think it is a wise idea to invoke the common
    code after name duplication checks.
    
    Best regards,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    
    

13. Re: ALTER command reworks

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-10-06T20:16:12Z

    2012/10/5 Alvaro Herrera <alvherre@2ndquadrant.com>:
    > Kohei KaiGai escribió:
    >> 2012/8/31 Kohei KaiGai <kaigai@kaigai.gr.jp>:
    >> > 2012/8/30 Robert Haas <robertmhaas@gmail.com>:
    >> >> On Mon, Aug 13, 2012 at 3:35 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    >
    >> >>> Was it a right decision to track dependency of large object using
    >> >>> oid of pg_largeobject, instead of pg_largeobject_metadata?
    >> >>> IIRC, the reason why we used oid of pg_largeobject is backward
    >> >>> compatibility for applications that tries to reference pg_depend
    >> >>> with built-in oids.
    >> >>
    >> >> I think it was a terrible decision and I'm pretty sure I said as much
    >> >> at the time, but I lost the argument, so I'm inclined to think we're
    >> >> stuck with continuing to support that kludge.
    >> >>
    >> > OK, we will keep to implement carefully...
    >
    > After reviewing this patch, I think we need to revisit this decision.
    >
    >> > OK, I'll split the patch into three (isn't it?) portions; RENAME, SET OWNER
    >> > and SET SCHEMA, with all above your suggestions.
    >> >
    >> As attached, I split off the original one into three portions; for set-schema,
    >> set-owner and rename-to. Please apply them in order of patch filename.
    >> Regarding to the error message, RenameErrorMsgAlreadyExists() was added
    >> to handle per object type messages in case when aclcheck_error() is not
    >> available to utilize.
    >
    > Here's the remaining piece; the RENAME part.  I have reworked it a bit,
    > but it needs a bit more work yet.  Note this:
    >
    > alvherre=# alter function foo.g(int, int) rename to g;
    > ERROR:  function foo.g(integer,integer) already exists in schema "foo"
    >
    > The previous code would have said
    >
    > alvherre=# alter function foo.g(int, int) rename to g;
    > ERROR:  function g(integer, integer) already exists in schema "foo"
    >
    > Note that the function name is not qualified here.  The difference is
    > that the original code was calling funcname_signature_string() to format
    > the function name, and the new code is calling format_procedure().  This
    > seems wrong to me; please rework.
    >
    > I haven't checked other object renames, but I think they should be okay
    > because functions are the only objects for which we pass the OID instead
    > of the name to the error message function.
    >
    The attached patch fixes the messaging issue.
    I newly add func_signature_string_oid() that returns compatible function's
    signature, but takes its object-id.
    
    So, the error message is now constructed as:
    +       case OBJECT_AGGREGATE:
    +       case OBJECT_FUNCTION:
    +           errorm = format_elog_string("function %s already exists in
    schema \"%s\"",
    +                                       func_signature_string_oid(objectId),
    +                                       get_namespace_name(namespaceId));
    +            break;
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    

14. Re: ALTER command reworks

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-10-17T20:09:24Z

    Kohei KaiGai escribió:
    > 2012/10/5 Alvaro Herrera <alvherre@2ndquadrant.com>:
    
    > The attached patch fixes the messaging issue.
    > I newly add func_signature_string_oid() that returns compatible function's
    > signature, but takes its object-id.
    > 
    > So, the error message is now constructed as:
    > +       case OBJECT_AGGREGATE:
    > +       case OBJECT_FUNCTION:
    > +           errorm = format_elog_string("function %s already exists in
    > schema \"%s\"",
    > +                                       func_signature_string_oid(objectId),
    > +                                       get_namespace_name(namespaceId));
    > +            break;
    
    Thanks, yeah, this works for me.
    
    I am now wondering if it would make sense to merge the duplicate-name
    error cases in AlterObjectNamespace_internal and
    AlterObjectRename_internal.  The former only works when there is a name
    catcache for the object type.  Maybe we can create a single function to
    which we give the object type, name/args, oid, etc, and it uses a
    catcache if available and falls back to get_object_address (with the
    IMO ugly name list manipulations) if not.
    
    -- 
    Álvaro Herrera                http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

15. Re: ALTER command reworks

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-11-15T18:44:46Z

    The attached patch is the revised version of ALTER RENAME TO
    consolidation. According to the previous suggestion, it uses
    a common logic to check object-naming duplication at
    check_duplicate_objectname().
    
    At the code path on AlterObjectNamespace_internal, it lost
    ObjectType information to the supplied object, so I also added
    get_object_type() function at objectaddress.c for inverse
    translation from a couple of classId/objectId to OBJECT_* label.
    
    Rest of parts are unchanged since the previous versions.
    
    Thanks,
    
    2012/10/17 Alvaro Herrera <alvherre@2ndquadrant.com>:
    > Kohei KaiGai escribió:
    >> 2012/10/5 Alvaro Herrera <alvherre@2ndquadrant.com>:
    >
    >> The attached patch fixes the messaging issue.
    >> I newly add func_signature_string_oid() that returns compatible function's
    >> signature, but takes its object-id.
    >>
    >> So, the error message is now constructed as:
    >> +       case OBJECT_AGGREGATE:
    >> +       case OBJECT_FUNCTION:
    >> +           errorm = format_elog_string("function %s already exists in
    >> schema \"%s\"",
    >> +                                       func_signature_string_oid(objectId),
    >> +                                       get_namespace_name(namespaceId));
    >> +            break;
    >
    > Thanks, yeah, this works for me.
    >
    > I am now wondering if it would make sense to merge the duplicate-name
    > error cases in AlterObjectNamespace_internal and
    > AlterObjectRename_internal.  The former only works when there is a name
    > catcache for the object type.  Maybe we can create a single function to
    > which we give the object type, name/args, oid, etc, and it uses a
    > catcache if available and falls back to get_object_address (with the
    > IMO ugly name list manipulations) if not.
    >
    > --
    > Įlvaro Herrera                http://www.2ndQuadrant.com/
    > PostgreSQL Development, 24x7 Support, Training & Services
    
    
    
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    

16. Re: ALTER command reworks

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2012-11-19T14:58:10Z

    Hi,
    
    Thanks for working on that cleanup job!
    
    Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
    > The attached patch is the revised version of ALTER RENAME TO
    > consolidation. According to the previous suggestion, it uses
    > a common logic to check object-naming duplication at
    > check_duplicate_objectname().
    
    I think you need to better comment which object types are supported by
    the new generic function and which are not in AlterObjectRename().
    
    > At the code path on AlterObjectNamespace_internal, it lost
    > ObjectType information to the supplied object, so I also added
    > get_object_type() function at objectaddress.c for inverse
    > translation from a couple of classId/objectId to OBJECT_* label.
    
    I don't understand that part, and it looks like it would be way better
    to find a way to pass down the information you already have earlier in
    the code than to compute it again doing syscache lookups…
    
    Regards,
    -- 
    Dimitri Fontaine
    http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support
    
    
    

17. Re: ALTER command reworks

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-11-19T15:25:09Z

    Hi Dimitri,
    
    Thanks for your checks.
    
    2012/11/19 Dimitri Fontaine <dimitri@2ndquadrant.fr>:
    > Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
    >> The attached patch is the revised version of ALTER RENAME TO
    >> consolidation. According to the previous suggestion, it uses
    >> a common logic to check object-naming duplication at
    >> check_duplicate_objectname().
    >
    > I think you need to better comment which object types are supported by
    > the new generic function and which are not in AlterObjectRename().
    >
    OK, Are you suggesting to add a "generic" comments such as "Generic
    function to change the name of a given object, for simple cases ...",
    not a list of OBJECT_* at the head of this function, aren't you?
    
    >> At the code path on AlterObjectNamespace_internal, it lost
    >> ObjectType information to the supplied object, so I also added
    >> get_object_type() function at objectaddress.c for inverse
    >> translation from a couple of classId/objectId to OBJECT_* label.
    >
    > I don't understand that part, and it looks like it would be way better
    > to find a way to pass down the information you already have earlier in
    > the code than to compute it again doing syscache lookups…
    >
    The pain point is AlterObjectNamespace_internal is not invoked by
    only ExecAlterObjectSchemaStmt(), but AlterObjectNamespace_oid()
    also.
    It is the reason why I had to put get_object_type() to find out OBJECT_*
    from the supplied ObjectAddress, because this code path does not
    available to pass down its ObjectType from the caller.
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    
    

18. Re: ALTER command reworks

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2012-11-19T15:39:27Z

    Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
    > OK, Are you suggesting to add a "generic" comments such as "Generic
    > function to change the name of a given object, for simple cases ...",
    > not a list of OBJECT_* at the head of this function, aren't you?
    
    Just something like
    
     * Most simple objects are covered by a generic function, the other
     * still need special processing.
    
    Mainly I was surprised to see collation not supported here, but I didn't
    take enough time to understand why that is the case. I will do that
    later in the review process.
    
    > The pain point is AlterObjectNamespace_internal is not invoked by
    > only ExecAlterObjectSchemaStmt(), but AlterObjectNamespace_oid()
    > also.
    
    I should remember better about that as the use case is extensions…
    
    > It is the reason why I had to put get_object_type() to find out OBJECT_*
    > from the supplied ObjectAddress, because this code path does not
    > available to pass down its ObjectType from the caller.
    
    If we really want to do that, I think I would only do that in
    AlterObjectNamespace_oid and add an argument to
    AlterObjectNamespace_internal, that you already have in
    ExecAlterObjectSchemaStmt().
    
    But really, what about just removing that part of your patch instead:
    
    @@ -396,14 +614,23 @@ AlterObjectNamespace_internal(Relation rel, Oid objid, Oid nspOid)
     	 * Check for duplicate name (more friendly than unique-index failure).
     	 * Since this is just a friendliness check, we can just skip it in cases
     	 * where there isn't a suitable syscache available.
    +	 *
    +	 * XXX - the caller should check object-name duplication, if the supplied
    +	 * object type need to take object arguments for identification, such as
    +	 * functions.
     	 */
    -	if (nameCacheId >= 0 &&
    -		SearchSysCacheExists2(nameCacheId, name, ObjectIdGetDatum(nspOid)))
    -		ereport(ERROR,
    -				(errcode(ERRCODE_DUPLICATE_OBJECT),
    -				 errmsg("%s already exists in schema \"%s\"",
    -						getObjectDescriptionOids(classId, objid),
    -						get_namespace_name(nspOid))));
    +	if (get_object_catcache_name(classId) >= 0)
    +	{
    +		ObjectAddress	address;
    +
    +		address.classId = classId;
    +		address.objectId = objid;
    +		address.objectSubId = 0;
    +
    +		objtype = get_object_type(&address);
    +		check_duplicate_objectname(objtype, nspOid,
    +								   NameStr(*DatumGetName(name)), NIL);
    +	}
    
    It would be much simpler to retain the old-style duplicate object check,
    and compared to doing extra cache lookups, it'd still be much cleaner in
    my view.
    
    Regards,
    -- 
    Dimitri Fontaine
    http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support
    
    
    

19. Re: ALTER command reworks

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-11-19T16:34:33Z

    2012/11/19 Dimitri Fontaine <dimitri@2ndquadrant.fr>:
    > Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
    >> OK, Are you suggesting to add a "generic" comments such as "Generic
    >> function to change the name of a given object, for simple cases ...",
    >> not a list of OBJECT_* at the head of this function, aren't you?
    >
    > Just something like
    >
    >  * Most simple objects are covered by a generic function, the other
    >  * still need special processing.
    >
    > Mainly I was surprised to see collation not supported here, but I didn't
    > take enough time to understand why that is the case. I will do that
    > later in the review process.
    >
    The reason why collation is not supported is that takes special name-
    duplication checks. The new collation name must have no collision on
    both of current database encoding and "any" encoding.
    It might be an idea to have a simple rule similar to
    AlterObjectNamespace_internal(); that requires caller to check
    namespace-duplication, if the given object type has no catcache-id
    with name-key.
    
    >> The pain point is AlterObjectNamespace_internal is not invoked by
    >> only ExecAlterObjectSchemaStmt(), but AlterObjectNamespace_oid()
    >> also.
    >
    > I should remember better about that as the use case is extensions…
    >
    >> It is the reason why I had to put get_object_type() to find out OBJECT_*
    >> from the supplied ObjectAddress, because this code path does not
    >> available to pass down its ObjectType from the caller.
    >
    > If we really want to do that, I think I would only do that in
    > AlterObjectNamespace_oid and add an argument to
    > AlterObjectNamespace_internal, that you already have in
    > ExecAlterObjectSchemaStmt().
    >
    > But really, what about just removing that part of your patch instead:
    >
    > @@ -396,14 +614,23 @@ AlterObjectNamespace_internal(Relation rel, Oid objid, Oid nspOid)
    >          * Check for duplicate name (more friendly than unique-index failure).
    >          * Since this is just a friendliness check, we can just skip it in cases
    >          * where there isn't a suitable syscache available.
    > +        *
    > +        * XXX - the caller should check object-name duplication, if the supplied
    > +        * object type need to take object arguments for identification, such as
    > +        * functions.
    >          */
    > -       if (nameCacheId >= 0 &&
    > -               SearchSysCacheExists2(nameCacheId, name, ObjectIdGetDatum(nspOid)))
    > -               ereport(ERROR,
    > -                               (errcode(ERRCODE_DUPLICATE_OBJECT),
    > -                                errmsg("%s already exists in schema \"%s\"",
    > -                                               getObjectDescriptionOids(classId, objid),
    > -                                               get_namespace_name(nspOid))));
    > +       if (get_object_catcache_name(classId) >= 0)
    > +       {
    > +               ObjectAddress   address;
    > +
    > +               address.classId = classId;
    > +               address.objectId = objid;
    > +               address.objectSubId = 0;
    > +
    > +               objtype = get_object_type(&address);
    > +               check_duplicate_objectname(objtype, nspOid,
    > +                                                                  NameStr(*DatumGetName(name)), NIL);
    > +       }
    >
    > It would be much simpler to retain the old-style duplicate object check,
    > and compared to doing extra cache lookups, it'd still be much cleaner in
    > my view.
    >
    Now, I get inclined to follow the manner of AlterObjectNamespace_internal
    at AlterObjectRename also; that requires caller to check name duplication
    in case when no catcache entry is supported.
    
    That simplifies the logic to check name duplication, and allows to eliminate
    get_object_type() here, even though RenameAggregate and
    RenameFunction have to be remained.
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    
    

20. Re: ALTER command reworks

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-11-30T22:07:23Z

    2012/11/19 Kohei KaiGai <kaigai@kaigai.gr.jp>:
    > 2012/11/19 Dimitri Fontaine <dimitri@2ndquadrant.fr>:
    >> Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
    >>> OK, Are you suggesting to add a "generic" comments such as "Generic
    >>> function to change the name of a given object, for simple cases ...",
    >>> not a list of OBJECT_* at the head of this function, aren't you?
    >>
    >> Just something like
    >>
    >>  * Most simple objects are covered by a generic function, the other
    >>  * still need special processing.
    >>
    >> Mainly I was surprised to see collation not supported here, but I didn't
    >> take enough time to understand why that is the case. I will do that
    >> later in the review process.
    >>
    > The reason why collation is not supported is that takes special name-
    > duplication checks. The new collation name must have no collision on
    > both of current database encoding and "any" encoding.
    > It might be an idea to have a simple rule similar to
    > AlterObjectNamespace_internal(); that requires caller to check
    > namespace-duplication, if the given object type has no catcache-id
    > with name-key.
    >
    >>> The pain point is AlterObjectNamespace_internal is not invoked by
    >>> only ExecAlterObjectSchemaStmt(), but AlterObjectNamespace_oid()
    >>> also.
    >>
    >> I should remember better about that as the use case is extensions…
    >>
    >>> It is the reason why I had to put get_object_type() to find out OBJECT_*
    >>> from the supplied ObjectAddress, because this code path does not
    >>> available to pass down its ObjectType from the caller.
    >>
    >> If we really want to do that, I think I would only do that in
    >> AlterObjectNamespace_oid and add an argument to
    >> AlterObjectNamespace_internal, that you already have in
    >> ExecAlterObjectSchemaStmt().
    >>
    >> But really, what about just removing that part of your patch instead:
    >>
    >> @@ -396,14 +614,23 @@ AlterObjectNamespace_internal(Relation rel, Oid objid, Oid nspOid)
    >>          * Check for duplicate name (more friendly than unique-index failure).
    >>          * Since this is just a friendliness check, we can just skip it in cases
    >>          * where there isn't a suitable syscache available.
    >> +        *
    >> +        * XXX - the caller should check object-name duplication, if the supplied
    >> +        * object type need to take object arguments for identification, such as
    >> +        * functions.
    >>          */
    >> -       if (nameCacheId >= 0 &&
    >> -               SearchSysCacheExists2(nameCacheId, name, ObjectIdGetDatum(nspOid)))
    >> -               ereport(ERROR,
    >> -                               (errcode(ERRCODE_DUPLICATE_OBJECT),
    >> -                                errmsg("%s already exists in schema \"%s\"",
    >> -                                               getObjectDescriptionOids(classId, objid),
    >> -                                               get_namespace_name(nspOid))));
    >> +       if (get_object_catcache_name(classId) >= 0)
    >> +       {
    >> +               ObjectAddress   address;
    >> +
    >> +               address.classId = classId;
    >> +               address.objectId = objid;
    >> +               address.objectSubId = 0;
    >> +
    >> +               objtype = get_object_type(&address);
    >> +               check_duplicate_objectname(objtype, nspOid,
    >> +                                                                  NameStr(*DatumGetName(name)), NIL);
    >> +       }
    >>
    >> It would be much simpler to retain the old-style duplicate object check,
    >> and compared to doing extra cache lookups, it'd still be much cleaner in
    >> my view.
    >>
    > Now, I get inclined to follow the manner of AlterObjectNamespace_internal
    > at AlterObjectRename also; that requires caller to check name duplication
    > in case when no catcache entry is supported.
    >
    > That simplifies the logic to check name duplication, and allows to eliminate
    > get_object_type() here, even though RenameAggregate and
    > RenameFunction have to be remained.
    >
    The attached patch is a revised version.
    
    It follows the manner in ExecAlterObjectSchemaStmt(); that tries to check
    name duplication for object classes that support catcache with name-key.
    Elsewhere, it calls individual routines for each object class to solve object
    name and check namespace conflicts.
    For example, RenameFunction() is still called from ExecRenameStmt(),
    however, it looks up the given function name and checks namespace
    conflict only, then it just invokes AlterObjectRename_internal() in spite
    of system catalog updates by itself.
    It allows to use this consolidated routine by object classes with special
    rule for namespace conflicts, such as collation.
    In addition, this design allowed to eliminate get_object_type() to pull
    OBJECT_* enum from class_id/object_id pair.
    
    Please check this patch.
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    

21. Re: ALTER command reworks

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2012-12-03T10:27:40Z

    Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
    > The attached patch is a revised version.
    
    I think you fixed all the problems I could see with your patch. It
    applies cleanly (with some offsets), compiles cleanly (I have a custom
    Makefile with CUSTOM_COPT=-Werror) and passes regression tests.
    
    I'll go mark it as ready for commiter. Thanks!
    
    > It follows the manner in ExecAlterObjectSchemaStmt(); that tries to check
    > name duplication for object classes that support catcache with name-key.
    > Elsewhere, it calls individual routines for each object class to solve object
    > name and check namespace conflicts.
    > For example, RenameFunction() is still called from ExecRenameStmt(),
    > however, it looks up the given function name and checks namespace
    > conflict only, then it just invokes AlterObjectRename_internal() in spite
    > of system catalog updates by itself.
    
    I think that's much better this way.
    
    > It allows to use this consolidated routine by object classes with special
    > rule for namespace conflicts, such as collation.
    > In addition, this design allowed to eliminate get_object_type() to pull
    > OBJECT_* enum from class_id/object_id pair.
    
    Thanks for having done this refactoring.
    
    Regards,
    -- 
    Dimitri Fontaine
    http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support
    
    
    

22. Re: ALTER command reworks

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2013-01-07T20:43:28Z

    Kohei KaiGai escribió:
    
    > The attached patch is a revised version.
    > 
    > It follows the manner in ExecAlterObjectSchemaStmt(); that tries to check
    > name duplication for object classes that support catcache with name-key.
    > Elsewhere, it calls individual routines for each object class to solve object
    > name and check namespace conflicts.
    > For example, RenameFunction() is still called from ExecRenameStmt(),
    > however, it looks up the given function name and checks namespace
    > conflict only, then it just invokes AlterObjectRename_internal() in spite
    > of system catalog updates by itself.
    > It allows to use this consolidated routine by object classes with special
    > rule for namespace conflicts, such as collation.
    > In addition, this design allowed to eliminate get_object_type() to pull
    > OBJECT_* enum from class_id/object_id pair.
    
    I checked this patch.  It needed a rebase for the changes to return
    OIDs.  Attached patch applies to current HEAD.  In general this looks
    good, with one exception: it's using getObjectDescriptionOids() to build
    the messages to complain in case the object already exists in the
    current schema, which results in diffs like this:
    
    -ERROR:  event trigger "regress_event_trigger2" already exists
    +ERROR:  event trigger regress_event_trigger2 already exists
    
    I don't know how tense we are about keeping the quotes, but I fear there
    would be complaints because it took us lots of sweat, blood and tears to
    get where we are now.
    
    If this is considered a problem, I think the way to fix it is to have a
    getObjectDescriptionOids() variant that quotes the object name in the
    output.  This would be pretty intrusive: we'd have to change things
    so that, for instance,
    
    		appendStringInfo(&buffer, _("collation %s"),
    			NameStr(coll->collname));
    
    would become
    
    		if (quotes)
    			appendStringInfo(&buffer, _("collation \"%s\""),
    				NameStr(coll->collname));
    		else
    			appendStringInfo(&buffer, _("collation %s"),
    				NameStr(coll->collname));
    
    Not really thrilled with this idea.  Of course,
    getObjectDescription() itself should keep the same API as now, to avoid
    useless churn all over the place.
    
    -- 
    Álvaro Herrera                http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
    

23. Re: ALTER command reworks

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

    On Mon, Jan 7, 2013 at 3:43 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
    > Kohei KaiGai escribió:
    >
    >> The attached patch is a revised version.
    >>
    >> It follows the manner in ExecAlterObjectSchemaStmt(); that tries to check
    >> name duplication for object classes that support catcache with name-key.
    >> Elsewhere, it calls individual routines for each object class to solve object
    >> name and check namespace conflicts.
    >> For example, RenameFunction() is still called from ExecRenameStmt(),
    >> however, it looks up the given function name and checks namespace
    >> conflict only, then it just invokes AlterObjectRename_internal() in spite
    >> of system catalog updates by itself.
    >> It allows to use this consolidated routine by object classes with special
    >> rule for namespace conflicts, such as collation.
    >> In addition, this design allowed to eliminate get_object_type() to pull
    >> OBJECT_* enum from class_id/object_id pair.
    >
    > I checked this patch.  It needed a rebase for the changes to return
    > OIDs.  Attached patch applies to current HEAD.  In general this looks
    > good, with one exception: it's using getObjectDescriptionOids() to build
    > the messages to complain in case the object already exists in the
    > current schema, which results in diffs like this:
    >
    > -ERROR:  event trigger "regress_event_trigger2" already exists
    > +ERROR:  event trigger regress_event_trigger2 already exists
    >
    > I don't know how tense we are about keeping the quotes, but I fear there
    > would be complaints because it took us lots of sweat, blood and tears to
    > get where we are now.
    >
    > If this is considered a problem, I think the way to fix it is to have a
    > getObjectDescriptionOids() variant that quotes the object name in the
    > output.  This would be pretty intrusive: we'd have to change things
    > so that, for instance,
    >
    >                 appendStringInfo(&buffer, _("collation %s"),
    >                         NameStr(coll->collname));
    >
    > would become
    >
    >                 if (quotes)
    >                         appendStringInfo(&buffer, _("collation \"%s\""),
    >                                 NameStr(coll->collname));
    >                 else
    >                         appendStringInfo(&buffer, _("collation %s"),
    >                                 NameStr(coll->collname));
    >
    > Not really thrilled with this idea.  Of course,
    > getObjectDescription() itself should keep the same API as now, to avoid
    > useless churn all over the place.
    
    This sort of thing has been rejected repeatedly in the past on
    translation grounds:
    
    + 				ereport(ERROR,
    + 						(errcode(ERRCODE_DUPLICATE_OBJECT),
    + 						 errmsg("%s already exists in schema \"%s\"",
    + 								getObjectDescriptionOids(classId, exists),
    + 								get_namespace_name(namespaceId))));
    
    If we're going to start allowing that, there's plenty of other code
    that can be hit with the same hammer, but I'm pretty sure that all
    previous proposals in this area have been slapped down.
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
    
    

24. Re: ALTER command reworks

    Tom Lane <tgl@sss.pgh.pa.us> — 2013-01-07T21:06:23Z

    Robert Haas <robertmhaas@gmail.com> writes:
    > On Mon, Jan 7, 2013 at 3:43 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
    >> I checked this patch.  It needed a rebase for the changes to return
    >> OIDs.  Attached patch applies to current HEAD.  In general this looks
    >> good, with one exception: it's using getObjectDescriptionOids() to build
    >> the messages to complain in case the object already exists in the
    >> current schema, which results in diffs like this:
    >> 
    >> -ERROR:  event trigger "regress_event_trigger2" already exists
    >> +ERROR:  event trigger regress_event_trigger2 already exists
    >> 
    >> I don't know how tense we are about keeping the quotes, but I fear there
    >> would be complaints because it took us lots of sweat, blood and tears to
    >> get where we are now.
    >> 
    >> If this is considered a problem, I think the way to fix it is to have a
    >> getObjectDescriptionOids() variant that quotes the object name in the
    >> output.
    
    > This sort of thing has been rejected repeatedly in the past on
    > translation grounds:
    
    Yes.  I'm surprised Alvaro isn't well aware of the rules against trying
    to build error messages out of sentence fragments: see first item under
    http://www.postgresql.org/docs/devel/static/nls-programmer.html#NLS-GUIDELINES
    
    Presence or absence of quotes is the very least of this code's i18n
    problems.
    
    If we had no other choice, we might consider a workaround such as that
    suggested in "Assembling Error Messages"
    http://www.postgresql.org/docs/devel/static/error-style-guide.html#AEN98605
    but frankly I'm not convinced that this patch is attractive enough to
    justify a degradation in message readability.
    
    			regards, tom lane
    
    
    

25. Re: ALTER command reworks

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2013-01-07T21:14:32Z

    2013/1/7 Tom Lane <tgl@sss.pgh.pa.us>:
    > Robert Haas <robertmhaas@gmail.com> writes:
    >> On Mon, Jan 7, 2013 at 3:43 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
    >>> I checked this patch.  It needed a rebase for the changes to return
    >>> OIDs.  Attached patch applies to current HEAD.  In general this looks
    >>> good, with one exception: it's using getObjectDescriptionOids() to build
    >>> the messages to complain in case the object already exists in the
    >>> current schema, which results in diffs like this:
    >>>
    >>> -ERROR:  event trigger "regress_event_trigger2" already exists
    >>> +ERROR:  event trigger regress_event_trigger2 already exists
    >>>
    >>> I don't know how tense we are about keeping the quotes, but I fear there
    >>> would be complaints because it took us lots of sweat, blood and tears to
    >>> get where we are now.
    >>>
    >>> If this is considered a problem, I think the way to fix it is to have a
    >>> getObjectDescriptionOids() variant that quotes the object name in the
    >>> output.
    >
    >> This sort of thing has been rejected repeatedly in the past on
    >> translation grounds:
    >
    > Yes.  I'm surprised Alvaro isn't well aware of the rules against trying
    > to build error messages out of sentence fragments: see first item under
    > http://www.postgresql.org/docs/devel/static/nls-programmer.html#NLS-GUIDELINES
    >
    > Presence or absence of quotes is the very least of this code's i18n
    > problems.
    >
    > If we had no other choice, we might consider a workaround such as that
    > suggested in "Assembling Error Messages"
    > http://www.postgresql.org/docs/devel/static/error-style-guide.html#AEN98605
    > but frankly I'm not convinced that this patch is attractive enough to
    > justify a degradation in message readability.
    >
    Sorry, I forgot Robert pointed out same thing before.
    I'll reconstruct the portion to raise an error message.
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    
    

26. Re: ALTER command reworks

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2013-01-07T21:58:52Z

    Tom Lane escribió:
    > Robert Haas <robertmhaas@gmail.com> writes:
    > > On Mon, Jan 7, 2013 at 3:43 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
    
    > >> If this is considered a problem, I think the way to fix it is to have a
    > >> getObjectDescriptionOids() variant that quotes the object name in the
    > >> output.
    > 
    > > This sort of thing has been rejected repeatedly in the past on
    > > translation grounds:
    > 
    > Yes.  I'm surprised Alvaro isn't well aware of the rules against trying
    > to build error messages out of sentence fragments: see first item under
    > http://www.postgresql.org/docs/devel/static/nls-programmer.html#NLS-GUIDELINES
    
    Actually I'm fully aware of the limitations; I just had a brain fade.  I
    knew there was something wrong with that usage of getObjectDescription,
    but managed to miss what it was exactly.  Doh.  Thankfully there are
    other hackers paying attention.
    
    BTW this patch was correct in this regard in a previous version -- there
    was one separate copy of the error message per object type.  I think
    it's just a matter of returning to that older coding.
    
    -- 
    Álvaro Herrera                http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

27. Re: ALTER command reworks

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2013-01-09T11:46:37Z

    2013/1/7 Alvaro Herrera <alvherre@2ndquadrant.com>:
    > Tom Lane escribió:
    >> Robert Haas <robertmhaas@gmail.com> writes:
    >> > On Mon, Jan 7, 2013 at 3:43 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
    >
    >> >> If this is considered a problem, I think the way to fix it is to have a
    >> >> getObjectDescriptionOids() variant that quotes the object name in the
    >> >> output.
    >>
    >> > This sort of thing has been rejected repeatedly in the past on
    >> > translation grounds:
    >>
    >> Yes.  I'm surprised Alvaro isn't well aware of the rules against trying
    >> to build error messages out of sentence fragments: see first item under
    >> http://www.postgresql.org/docs/devel/static/nls-programmer.html#NLS-GUIDELINES
    >
    > Actually I'm fully aware of the limitations; I just had a brain fade.  I
    > knew there was something wrong with that usage of getObjectDescription,
    > but managed to miss what it was exactly.  Doh.  Thankfully there are
    > other hackers paying attention.
    >
    > BTW this patch was correct in this regard in a previous version -- there
    > was one separate copy of the error message per object type.  I think
    > it's just a matter of returning to that older coding.
    >
    The attached patch is a rebased version towards the latest master
    branch, and fix up the issue around error messages on name conflicts.
    
    I could also find an abused of getObjectDescription() for error message
    of namespace conflicts at AlterObjectNamespace_internal().
    The newly added ereport_on_namespace_conflict() can handle the
    both of cases, so I also replaced it.
    
    As we have a discussion in another thread, we may add special case
    handling for functions and collation on checks on namespace conflicts,
    however, I didn't tough code path of them, right now.
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    

28. Re: ALTER command reworks

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2013-01-15T15:06:12Z

    Kohei KaiGai escribió:
    
    > The attached patch is a rebased version towards the latest master
    > branch, and fix up the issue around error messages on name conflicts.
    
    I assume the lock.c changes are just a bollixed merge, right?
    
    I am not sure about some of the changes in the regression test expected
    output; are we really okay with losing the "in schema foo" part in some
    of these cases?
    
    -- 
    Álvaro Herrera                http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

29. Re: ALTER command reworks

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2013-01-15T15:40:29Z

    2013/1/15 Alvaro Herrera <alvherre@2ndquadrant.com>:
    > Kohei KaiGai escribió:
    >
    >> The attached patch is a rebased version towards the latest master
    >> branch, and fix up the issue around error messages on name conflicts.
    >
    > I assume the lock.c changes are just a bollixed merge, right?
    >
    Yes, I'll check it and rebase it.
    
    > I am not sure about some of the changes in the regression test expected
    > output; are we really okay with losing the "in schema foo" part in some
    > of these cases?
    >
    The changes in the expected results in regression test originated from
    elimination of getObjectDescription, but "in schema foo" should be kept.
    It looks like an obvious my mistake. Sorry.
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    
    

30. Re: ALTER command reworks

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2013-01-15T15:49:52Z

    Kohei KaiGai escribió:
    > 2013/1/15 Alvaro Herrera <alvherre@2ndquadrant.com>:
    > > Kohei KaiGai escribió:
    > >
    > >> The attached patch is a rebased version towards the latest master
    > >> branch, and fix up the issue around error messages on name conflicts.
    > >
    > > I assume the lock.c changes are just a bollixed merge, right?
    > >
    > Yes, I'll check it and rebase it.
    
    Wait for a bit before publishing a new version -- I'm going to push the
    other patch so that you can merge on top.
    
    Note that I'm going to commit a new function like this:
    
    /*
     * Raise an error to the effect that an object of the given name is already
     * present in the given namespace.
     */
    static void
    report_namespace_conflict(Oid classId, const char *name, Oid nspOid)
    {
    	char   *msgfmt;
    
    	Assert(OidIsValid(nspOid));
    
    
    For this patch you will need to create a separate function which does
    the conflicting-name report for objects that are not in a namespace.
    Mixing both in-schema and schemaless objects in the same report function
    seems messy to me.
    
    -- 
    Álvaro Herrera                http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

31. Re: ALTER command reworks

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2013-01-17T00:13:22Z

    2013/1/15 Alvaro Herrera <alvherre@2ndquadrant.com>:
    > Kohei KaiGai escribió:
    >> 2013/1/15 Alvaro Herrera <alvherre@2ndquadrant.com>:
    >> > Kohei KaiGai escribió:
    >> >
    >> >> The attached patch is a rebased version towards the latest master
    >> >> branch, and fix up the issue around error messages on name conflicts.
    >> >
    >> > I assume the lock.c changes are just a bollixed merge, right?
    >> >
    >> Yes, I'll check it and rebase it.
    >
    > Wait for a bit before publishing a new version -- I'm going to push the
    > other patch so that you can merge on top.
    >
    > Note that I'm going to commit a new function like this:
    >
    > /*
    >  * Raise an error to the effect that an object of the given name is already
    >  * present in the given namespace.
    >  */
    > static void
    > report_namespace_conflict(Oid classId, const char *name, Oid nspOid)
    > {
    >         char   *msgfmt;
    >
    >         Assert(OidIsValid(nspOid));
    >
    >
    > For this patch you will need to create a separate function which does
    > the conflicting-name report for objects that are not in a namespace.
    > Mixing both in-schema and schemaless objects in the same report function
    > seems messy to me.
    >
    This attached patch is the rebased one towards the latest master branch.
    
    I noticed that your proposed design also allows to unify ALTER code of
    OPERATOR CLASS / FAMILY; that takes index access method for its
    namespace in addition to name and namespace.
    So, AlterObjectRename_internal and AlterObjectNamespace_internal have
    four of special case handling to check name / namespace conflict.
    
    The latest master lookups syscache within special case handing block
    as follows:
    
        if (classId == ProcedureRelationId)
        {
            HeapTuple   tup;
            Form_pg_proc proc;
    
            tup = SearchSysCacheCopy1(PROCOID, ObjectIdGetDatum(objid));
            if (!HeapTupleIsValid(tup))
                elog(ERROR, "cache lookup failed for function %u", objid);
            proc = (Form_pg_proc) GETSTRUCT(tup);
    
            IsThereFunctionInNamespace(NameStr(proc->proname), proc->pronargs,
                                       proc->proargtypes, nspOid);
            heap_freetuple(tup);
        }
    
    But, we already pulls a relevant tuple from syscache on top of
    AlterObjectNamespace_internal. So, I removed cache lookup code here.
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    

32. Re: ALTER command reworks

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2013-01-17T21:59:03Z

    Kohei KaiGai escribió:
    
    > This attached patch is the rebased one towards the latest master branch.
    
    Great, thanks.  I played with it a bit and it looks almost done to me.
    The only issue I can find is that it lets you rename an aggregate by
    using ALTER FUNCTION, which is supposed to be forbidden.  (Funnily
    enough, renaming a non-agg function with ALTER AGGREGATE does raise an
    error).  Didn't immediately spot the right place to add a check.
    
    I think these two error cases ought to have regression tests of their
    own.
    
    I attach a version with my changes.
    
    > I noticed that your proposed design also allows to unify ALTER code of
    > OPERATOR CLASS / FAMILY; that takes index access method for its
    > namespace in addition to name and namespace.
    
    Yeah, I had noticed that and was planning on implementing it later.
    Thanks for saving me the work.
    
    > So, AlterObjectRename_internal and AlterObjectNamespace_internal have
    > four of special case handling to check name / namespace conflict.
    
    Right.  (I wonder if it would make sense to encapsulate all those checks
    in a single function for both to call.)
    
    > The latest master lookups syscache within special case handing block
    > as follows:
    > [code]
    > 
    > But, we already pulls a relevant tuple from syscache on top of
    > AlterObjectNamespace_internal. So, I removed cache lookup code here.
    
    Silly me.  Thanks.
    
    -- 
    Álvaro Herrera                http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
    

33. Re: ALTER command reworks

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2013-01-19T22:48:50Z

    2013/1/17 Alvaro Herrera <alvherre@2ndquadrant.com>:
    > Kohei KaiGai escribió:
    >
    >> This attached patch is the rebased one towards the latest master branch.
    >
    > Great, thanks.  I played with it a bit and it looks almost done to me.
    > The only issue I can find is that it lets you rename an aggregate by
    > using ALTER FUNCTION, which is supposed to be forbidden.  (Funnily
    > enough, renaming a non-agg function with ALTER AGGREGATE does raise an
    > error).  Didn't immediately spot the right place to add a check.
    >
    > I think these two error cases ought to have regression tests of their
    > own.
    >
    > I attach a version with my changes.
    >
    The patch itself looks to me good.
    
    About ALTER FUNCTION towards aggregate function, why we should raise
    an error strictly? Some code allows to modify properties of aggregate function
    specified with FUNCTION qualifier.
    
    postgres=# COMMENT ON FUNCTION max(int) IS 'maximum number of integer';
    COMMENT
    postgres=# COMMENT ON AGGREGATE in4eq(int,int) IS 'comparison of integers';
    ERROR:  aggregate in4eq(integer, integer) does not exist
    
    I think using AGGREGATE towards regular function is wrong, but it is not
    100% wrong to use FUNCTION towards aggregate "function".
    In addition, aggregate function and regular function share same namespace,
    so it never makes problem even if we allows to identify aggregate function
    using ALTER FUNCTION.
    
    How about your opinion? Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    
    

34. Re: ALTER command reworks

    Tom Lane <tgl@sss.pgh.pa.us> — 2013-01-20T16:28:24Z

    Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
    > About ALTER FUNCTION towards aggregate function, why we should raise
    > an error strictly?
    
    I agree we probably shouldn't --- traditionally we have allowed that,
    AFAIR, so changing it would break existing applications for little
    benefit.
    
    Similarly, you should not be throwing error when ALTER TABLE is applied
    to a view, sequence, etc, and the command would otherwise be sensible.
    
    			regards, tom lane
    
    
    

35. Re: ALTER command reworks

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2013-01-21T15:12:16Z

    Tom Lane escribió:
    > Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
    > > About ALTER FUNCTION towards aggregate function, why we should raise
    > > an error strictly?
    > 
    > I agree we probably shouldn't --- traditionally we have allowed that,
    > AFAIR, so changing it would break existing applications for little
    > benefit.
    
    Okay, I have pushed the version I posted last week.
    
    > Similarly, you should not be throwing error when ALTER TABLE is applied
    > to a view, sequence, etc, and the command would otherwise be sensible.
    
    As far as ALTER some-obj RENAME goes, this is already working, so I
    haven't changed anything.
    
    Thanks,
    
    -- 
    Álvaro Herrera                http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

36. Re: ALTER command reworks

    Tom Lane <tgl@sss.pgh.pa.us> — 2013-02-03T21:57:27Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
    > [ pgsql-v9.3-alter-reworks.3-rename.v10.patch.gz ]
    
    Say ... I hadn't been paying too close attention to this patch, but
    is there any particularly principled reason for it having unified
    only 14 of the 29 object types handled by ExecRenameStmt()?
    If so, how to tell which object types are supposed to be covered?
    
    The reason I'm asking is that it's very unclear to me whether
    https://commitfest.postgresql.org/action/patch_view?id=1043
    (ALTER RENAME RULE) is okay in more-or-less its current form,
    or whether it ought to be bounced back to be reworked for integration
    in this framework.
    
    			regards, tom lane
    
    
    

37. Re: ALTER command reworks

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2013-02-04T05:02:24Z

    2013/2/3 Tom Lane <tgl@sss.pgh.pa.us>:
    > Alvaro Herrera <alvherre@2ndquadrant.com> writes:
    >> [ pgsql-v9.3-alter-reworks.3-rename.v10.patch.gz ]
    >
    > Say ... I hadn't been paying too close attention to this patch, but
    > is there any particularly principled reason for it having unified
    > only 14 of the 29 object types handled by ExecRenameStmt()?
    > If so, how to tell which object types are supposed to be covered?
    >
    > The reason I'm asking is that it's very unclear to me whether
    > https://commitfest.postgresql.org/action/patch_view?id=1043
    > (ALTER RENAME RULE) is okay in more-or-less its current form,
    > or whether it ought to be bounced back to be reworked for integration
    > in this framework.
    >
    Like trigger or constraint, rule is unavailable to integrate the generic
    rename logic using AlterObjectRename_internal().
    So, I don't think this patch needs to take much design change.
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    
    

38. Re: ALTER command reworks

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2013-02-04T14:23:31Z

    Kohei KaiGai escribió:
    > 2013/2/3 Tom Lane <tgl@sss.pgh.pa.us>:
    > > Alvaro Herrera <alvherre@2ndquadrant.com> writes:
    > >> [ pgsql-v9.3-alter-reworks.3-rename.v10.patch.gz ]
    > >
    > > Say ... I hadn't been paying too close attention to this patch, but
    > > is there any particularly principled reason for it having unified
    > > only 14 of the 29 object types handled by ExecRenameStmt()?
    > > If so, how to tell which object types are supposed to be covered?
    > >
    > > The reason I'm asking is that it's very unclear to me whether
    > > https://commitfest.postgresql.org/action/patch_view?id=1043
    > > (ALTER RENAME RULE) is okay in more-or-less its current form,
    > > or whether it ought to be bounced back to be reworked for integration
    > > in this framework.
    > >
    > Like trigger or constraint, rule is unavailable to integrate the generic
    > rename logic using AlterObjectRename_internal().
    > So, I don't think this patch needs to take much design change.
    
    I did give that patch a glance last week, asked myself the same question
    as Tom, and gave myself the same answer as KaiGai.  Sorry I didn't post
    that.
    
    -- 
    Álvaro Herrera                http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services