Thread

1. Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement

   Kohei KaiGai <kaigai@kaigai.gr.jp> — 2011-11-15T09:43:54Z

   The series of attached patches try to have refactoring on several ddl
   statements; that consolidates distributed routines for each object
   types into a common routine to minimize the number of upcoming hooks
   for external security.
   Please apply the series of patches in order of part-1, part-2, part-3
   then part-4.
   
   Part-1) DROP statement refactoring
   It is a remaining portion of what I submitted in the last commit fest.
   It allows object types that didn't used DropStmt in gram.y to go
   through RemoveObjects(), instead of individual RemoveXXXX().
   
   Part-2) Groundworks on objectaddress.c
   This patch adds necessary groundworks for Part-3 and Part-4.
   It adds ObjectPropertyType of objectaddress.c index-oid and cache-id
   for name lookup and attribute number of object name; these field is
   used to detect namespace conflict on object_exists_namespace() that
   shall be called on refactored ALTER SET SCHEMA/RENAME TO.
   It also reduce some arguments of check_object_ownership(), and allows
   to call this function without knowledge of ObjectType and text
   representation. It allows to check ownership using this function from
   the code path of AlterObjectNamespace_oid() that does not have (or
   need to look up catalog to obtain) ObjectType information.
   In addition, it adds regression test cases for ALTER SET SCHEMA/RENAME TO.
   
   Part-3) ALTER SET SCHEMA statement refactoring
   This patch refactoring routines of ALTER SET SCHEMA implementations of
   most object types; except for relations, types and extensions. This
   portion was originally designed to use a common routine
   (AlterObjectNamespace) via wrapper function that delivers
   characteristic properties of object types, then this design was
   replaced by facilities of objectaddress.c.
   
   Part-4) ALTER RENAME TO statement refactoring
   This patch refactoring routines of ALTER RENAME TO implementation of
   most object types; except for databases, relations, columns, triggers
   and roles.
   
   Thanks,
   -- 
   KaiGai Kohei <kaigai@kaigai.gr.jp>
   

2. Re: Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement

   Robert Haas <robertmhaas@gmail.com> — 2011-11-17T16:21:09Z

   On Tue, Nov 15, 2011 at 4:43 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
   > Part-1) DROP statement refactoring
   > It is a remaining portion of what I submitted in the last commit fest.
   > It allows object types that didn't used DropStmt in gram.y to go
   > through RemoveObjects(), instead of individual RemoveXXXX().
   
   Review of just this part:
   
   - I think we can remove the special case for foreign data wrappers
   because (1) the only case in which there's any behavioral difference
   at all is if a superuser creates a foreign data wrapper (or the
   ownership of one is reassigned to him) and he is then made not a
   superuser; non-superusers can't create foreign data wrappers, and
   existing foreign data wrappers can't be given to non-superusers;
   moreover, (2) removing the special case causes the behavior to match
   the documentation, which it currently doesn't (but only in the
   aforementioned, extremely minor way).
   
   - On the other hand, this patch blithely nukes the prohibition on
   using DROP FUNCTION to remove an aggregate.  I'm not sure that's a
   good idea.  It also eliminates the NOTICE when removing a built-in
   function, which I think is OK because you don't actually get that far:
   
   rhaas=# drop function int4pl(integer, integer);
   ERROR:  cannot drop function int4pl(integer,integer) because it is
   required by the database system
   
   - For some reason, we have code that causes procedural language names
   to be downcased before use.  Given that unquoted identifiers are
   downcased anyway, this seems a bit redundant.  I'm inclined to think
   we don't need to preserve that behavior for DROP, especially because
   other parts of the code - such as COMMENT - don't know about it
   anyway.  But rather than just changing it for DROP, I think we should
   go through and rip out case_translate_language_name() across the
   board, probably as a a separate commit.
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   
   

3. Re: Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement

   Tom Lane <tgl@sss.pgh.pa.us> — 2011-11-17T18:00:25Z

   Robert Haas <robertmhaas@gmail.com> writes:
   > It also eliminates the NOTICE when removing a built-in
   > function, which I think is OK because you don't actually get that far:
   
   There are paths that can reach that notice --- I think what you have to
   do is create a new function that references a built-in one.  But why
   we bother to warn for that isn't clear to me.
   
   > - For some reason, we have code that causes procedural language names
   > to be downcased before use.
   
   I think this is a hangover from the fact that CREATE FUNCTION's LANGUAGE
   clause used to insist on the language name being a string literal, and
   of course the lexer didn't case-fold it then.  That's been deprecated
   for long enough that we probably don't need to have the extra case-fold
   step anymore.
   
   			regards, tom lane
   
   

4. Re: Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement

   Robert Haas <robertmhaas@gmail.com> — 2011-11-17T19:25:03Z

   On Thu, Nov 17, 2011 at 1:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
   > Robert Haas <robertmhaas@gmail.com> writes:
   >> It also eliminates the NOTICE when removing a built-in
   >> function, which I think is OK because you don't actually get that far:
   >
   > There are paths that can reach that notice --- I think what you have to
   > do is create a new function that references a built-in one.  But why
   > we bother to warn for that isn't clear to me.
   >
   >> - For some reason, we have code that causes procedural language names
   >> to be downcased before use.
   >
   > I think this is a hangover from the fact that CREATE FUNCTION's LANGUAGE
   > clause used to insist on the language name being a string literal, and
   > of course the lexer didn't case-fold it then.  That's been deprecated
   > for long enough that we probably don't need to have the extra case-fold
   > step anymore.
   
   OK, great.
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   
   

5. Re: Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement

   Alvaro Herrera <alvherre@commandprompt.com> — 2011-11-17T21:26:09Z

   Excerpts from Robert Haas's message of jue nov 17 16:25:03 -0300 2011:
   > On Thu, Nov 17, 2011 at 1:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
   > > Robert Haas <robertmhaas@gmail.com> writes:
   
   > >> - For some reason, we have code that causes procedural language names
   > >> to be downcased before use.
   > >
   > > I think this is a hangover from the fact that CREATE FUNCTION's LANGUAGE
   > > clause used to insist on the language name being a string literal, and
   > > of course the lexer didn't case-fold it then.  That's been deprecated
   > > for long enough that we probably don't need to have the extra case-fold
   > > step anymore.
   > 
   > OK, great.
   
   So the buildfarm broke due to this change, because citext does
   
   --
   -- Aggregates.
   --
   
   CREATE FUNCTION citext_smaller(citext, citext)
   RETURNS citext
   AS 'MODULE_PATHNAME'
   LANGUAGE 'C' IMMUTABLE STRICT;
   
   CREATE FUNCTION citext_larger(citext, citext)
   RETURNS citext
   AS 'MODULE_PATHNAME'
   LANGUAGE 'C' IMMUTABLE STRICT;
   
   
   -- 
   Álvaro Herrera <alvherre@commandprompt.com>
   The PostgreSQL Company - Command Prompt, Inc.
   PostgreSQL Replication, Consulting, Custom Development, 24x7 support
   
   

6. Re: Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement

   Robert Haas <robertmhaas@gmail.com> — 2011-11-17T22:14:30Z

   On Thu, Nov 17, 2011 at 4:26 PM, Alvaro Herrera
   <alvherre@commandprompt.com> wrote:
   > So the buildfarm broke due to this change, because citext does
   
   Thanks for fixing it.  Should we revert the original change?
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   
   

7. Re: Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement

   Tom Lane <tgl@sss.pgh.pa.us> — 2011-11-17T22:29:29Z

   Robert Haas <robertmhaas@gmail.com> writes:
   > On Thu, Nov 17, 2011 at 4:26 PM, Alvaro Herrera
   > <alvherre@commandprompt.com> wrote:
   >> So the buildfarm broke due to this change, because citext does
   
   > Thanks for fixing it.  Should we revert the original change?
   
   I still think it's reasonable to remove the extra downcasing step,
   but we'll have to document it as a change.  For instance, spelling
   C as either "C" or 'C' would work differently now.  The fact that
   the former is downcased seems quite surprising to me, so I don't
   think anybody would say that this isn't a better definition, but
   undoubtedly it could force people to change their source files.
   
   			regards, tom lane
   
   

8. Re: Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement

   Robert Haas <robertmhaas@gmail.com> — 2011-11-17T23:45:44Z

   On Thu, Nov 17, 2011 at 5:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
   > Robert Haas <robertmhaas@gmail.com> writes:
   >> On Thu, Nov 17, 2011 at 4:26 PM, Alvaro Herrera
   >> <alvherre@commandprompt.com> wrote:
   >>> So the buildfarm broke due to this change, because citext does
   >
   >> Thanks for fixing it.  Should we revert the original change?
   >
   > I still think it's reasonable to remove the extra downcasing step,
   > but we'll have to document it as a change.  For instance, spelling
   > C as either "C" or 'C' would work differently now.  The fact that
   > the former is downcased seems quite surprising to me, so I don't
   > think anybody would say that this isn't a better definition, but
   > undoubtedly it could force people to change their source files.
   
   So, should we add a note to all the LANGUAGE command pages in the
   manual?  Or just include this in the release notes?
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   
   

9. Re: Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement

   Tom Lane <tgl@sss.pgh.pa.us> — 2011-11-18T00:13:50Z

   Robert Haas <robertmhaas@gmail.com> writes:
   > On Thu, Nov 17, 2011 at 5:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
   >> I still think it's reasonable to remove the extra downcasing step,
   >> but we'll have to document it as a change.
   
   > So, should we add a note to all the LANGUAGE command pages in the
   > manual?  Or just include this in the release notes?
   
   Release note seems sufficient.  I looked at the text in CREATE FUNCTION
   and it doesn't seem to need changing.
   
   			regards, tom lane
   
   

10. Re: Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement

    Robert Haas <robertmhaas@gmail.com> — 2011-11-18T02:33:16Z

    On Thu, Nov 17, 2011 at 11:21 AM, Robert Haas <robertmhaas@gmail.com> wrote:
    > On Tue, Nov 15, 2011 at 4:43 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    >> Part-1) DROP statement refactoring
    >> It is a remaining portion of what I submitted in the last commit fest.
    >> It allows object types that didn't used DropStmt in gram.y to go
    >> through RemoveObjects(), instead of individual RemoveXXXX().
    >
    > Review of just this part:
    >
    > - I think we can remove the special case for foreign data wrappers
    > because (1) the only case in which there's any behavioral difference
    > at all is if a superuser creates a foreign data wrapper (or the
    > ownership of one is reassigned to him) and he is then made not a
    > superuser; non-superusers can't create foreign data wrappers, and
    > existing foreign data wrappers can't be given to non-superusers;
    > moreover, (2) removing the special case causes the behavior to match
    > the documentation, which it currently doesn't (but only in the
    > aforementioned, extremely minor way).
    >
    > - On the other hand, this patch blithely nukes the prohibition on
    > using DROP FUNCTION to remove an aggregate.  I'm not sure that's a
    > good idea.  It also eliminates the NOTICE when removing a built-in
    > function, which I think is OK because you don't actually get that far:
    >
    > rhaas=# drop function int4pl(integer, integer);
    > ERROR:  cannot drop function int4pl(integer,integer) because it is
    > required by the database system
    >
    > - For some reason, we have code that causes procedural language names
    > to be downcased before use.  Given that unquoted identifiers are
    > downcased anyway, this seems a bit redundant.  I'm inclined to think
    > we don't need to preserve that behavior for DROP, especially because
    > other parts of the code - such as COMMENT - don't know about it
    > anyway.  But rather than just changing it for DROP, I think we should
    > go through and rip out case_translate_language_name() across the
    > board, probably as a a separate commit.
    
    I've committed part 1 of this patch series after correcting the above items.
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
    

11. Re: Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement

    Robert Haas <robertmhaas@gmail.com> — 2011-11-18T03:43:24Z

    On Tue, Nov 15, 2011 at 4:43 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    > Part-2) Groundworks on objectaddress.c
    > This patch adds necessary groundworks for Part-3 and Part-4.
    > It adds ObjectPropertyType of objectaddress.c index-oid and cache-id
    > for name lookup and attribute number of object name; these field is
    > used to detect namespace conflict on object_exists_namespace() that
    > shall be called on refactored ALTER SET SCHEMA/RENAME TO.
    > It also reduce some arguments of check_object_ownership(), and allows
    > to call this function without knowledge of ObjectType and text
    > representation. It allows to check ownership using this function from
    > the code path of AlterObjectNamespace_oid() that does not have (or
    > need to look up catalog to obtain) ObjectType information.
    > In addition, it adds regression test cases for ALTER SET SCHEMA/RENAME TO.
    
    This part adds a new regression test to a parallel group with the
    following comments:
    
    # NB: temp.sql does a reconnect which transiently uses 2 connections,
    # so keep this parallel group to at most 19 tests
    
    ...and this would be test #20.  So we either need to move it
    elsewhere, or move something else elsewhere.  I'm tempted to add it to
    this rather scrawny-looking group, unless someone sees a reason to do
    otherwise:
    
    test: privileges security_label collate
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
    

12. Re: Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement

    Robert Haas <robertmhaas@gmail.com> — 2011-11-18T04:19:58Z

    On Tue, Nov 15, 2011 at 4:43 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    > Part-2) Groundworks on objectaddress.c
    > This patch adds necessary groundworks for Part-3 and Part-4.
    > It adds ObjectPropertyType of objectaddress.c index-oid and cache-id
    > for name lookup and attribute number of object name; these field is
    > used to detect namespace conflict on object_exists_namespace() that
    > shall be called on refactored ALTER SET SCHEMA/RENAME TO.
    > It also reduce some arguments of check_object_ownership(), and allows
    > to call this function without knowledge of ObjectType and text
    > representation. It allows to check ownership using this function from
    > the code path of AlterObjectNamespace_oid() that does not have (or
    > need to look up catalog to obtain) ObjectType information.
    
    I spent some time wading through the code for parts 2 through 4, and I
    guess I'm not sure this is headed in the right direction.  If we need
    this much infrastructure in order to consolidate the handling of ALTER
    BLAH .. SET SCHEMA, then maybe it's not worth doing.
    
    But I'm not sure why we do.  My thought here was that we should
    extended the ObjectProperty array in objectaddress.c so that
    AlterObjectNamespace can get by with fewer arguments - specifically,
    it seems like the following ought to be things that can be looked up
    via the ObjectProperty mechanism:
    
    int oidCacheId, int nameCacheId, int Anum_name, int Anum_namespace,
    int Anum_owner, AclObjectKind acl_kind
    
    The advantage of that, or so it seems to me, is that all this
    information is in a table in objectaddress.c where multiple clients
    can get at it at need, as opposed to the current system where it's
    passed as arguments to AlterObjectNamespace(), and all that would need
    to be duplicated if we had another function that did something
    similar.
    
    Now, what you have here is a much broader reworking.  And that's not
    necessarily bad, but at the moment I'm not really seeing how it
    benefits us.
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
    

13. Re: Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2011-11-19T18:49:29Z

    2011/11/18 Robert Haas <robertmhaas@gmail.com>:
    > On Tue, Nov 15, 2011 at 4:43 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    >> Part-2) Groundworks on objectaddress.c
    >> This patch adds necessary groundworks for Part-3 and Part-4.
    >> It adds ObjectPropertyType of objectaddress.c index-oid and cache-id
    >> for name lookup and attribute number of object name; these field is
    >> used to detect namespace conflict on object_exists_namespace() that
    >> shall be called on refactored ALTER SET SCHEMA/RENAME TO.
    >> It also reduce some arguments of check_object_ownership(), and allows
    >> to call this function without knowledge of ObjectType and text
    >> representation. It allows to check ownership using this function from
    >> the code path of AlterObjectNamespace_oid() that does not have (or
    >> need to look up catalog to obtain) ObjectType information.
    >
    > I spent some time wading through the code for parts 2 through 4, and I
    > guess I'm not sure this is headed in the right direction.  If we need
    > this much infrastructure in order to consolidate the handling of ALTER
    > BLAH .. SET SCHEMA, then maybe it's not worth doing.
    >
    > But I'm not sure why we do.  My thought here was that we should
    > extended the ObjectProperty array in objectaddress.c so that
    > AlterObjectNamespace can get by with fewer arguments - specifically,
    > it seems like the following ought to be things that can be looked up
    > via the ObjectProperty mechanism:
    >
    > int oidCacheId, int nameCacheId, int Anum_name, int Anum_namespace,
    > int Anum_owner, AclObjectKind acl_kind
    >
    Thanks for your reviewing, and sorry for not a timely response.
    
    I tried to add these items into ObjectProperty and replace existing caller of
    AlterObjectNamespace, however, it seemed to me these members (especially
    AclObjectKind) were too specific with current implementation of the
    AlterObjectNamespace.
    
    I think, SET SCHEMA, RENAME TO and OWNER TO are candidate to
    consolidate similar codes using facilities in objectaddress.c.
    Even though SET SCHEMA is mostly consolidated with AlterObjectNamespace,
    RENAME TO and OWNER TO are implemented individual routines for each
    object types. So, I thought it is preferable way to provide groundwork to be
    applied these routines also.
    
    In the part-2 patch, I added object_exists_namespace() to check namespace
    conflicts commonly used to SET SCHEMA and RENAME TO, although we
    have individual routines for RENAME TO.
    And, I also modified check_ownership() to eliminate objtype/object/objarg; that
    allows to invoke this function from code paths without these
    information, such as
    shdepReassignOwned() or AlterObjectNamespace_oid().
    
    > The advantage of that, or so it seems to me, is that all this
    > information is in a table in objectaddress.c where multiple clients
    > can get at it at need, as opposed to the current system where it's
    > passed as arguments to AlterObjectNamespace(), and all that would need
    > to be duplicated if we had another function that did something
    > similar.
    >
    Yes, correct.
    
    > Now, what you have here is a much broader reworking.  And that's not
    > necessarily bad, but at the moment I'm not really seeing how it
    > benefits us.
    >
    In my point, if individual object types need to have its own handler for
    alter commands, points of the code to check permissions are also
    distributed for each object types. It shall be a burden to maintain hooks
    that allows modules (e.g sepgsql) to have permission checks.
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    

14. Re: Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement

    Robert Haas <robertmhaas@gmail.com> — 2011-11-21T20:36:59Z

    On Sat, Nov 19, 2011 at 1:49 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    >> But I'm not sure why we do.  My thought here was that we should
    >> extended the ObjectProperty array in objectaddress.c so that
    >> AlterObjectNamespace can get by with fewer arguments - specifically,
    >> it seems like the following ought to be things that can be looked up
    >> via the ObjectProperty mechanism:
    >>
    >> int oidCacheId, int nameCacheId, int Anum_name, int Anum_namespace,
    >> int Anum_owner, AclObjectKind acl_kind
    >>
    > Thanks for your reviewing, and sorry for not a timely response.
    >
    > I tried to add these items into ObjectProperty and replace existing caller of
    > AlterObjectNamespace, however, it seemed to me these members (especially
    > AclObjectKind) were too specific with current implementation of the
    > AlterObjectNamespace.
    
    Hmm, maybe so.  But then we could still move over some things.
    oidCacheId is pretty much already there already, isn't it?
    
    > And, I also modified check_ownership() to eliminate objtype/object/objarg; that
    > allows to invoke this function from code paths without these
    > information, such as
    > shdepReassignOwned() or AlterObjectNamespace_oid().
    
    Yeah.  I'm not sure I like that.  It doesn't seem like a particularly
    good idea to throw away the information we have about the name the
    user entered and assume we'll be able to regenerate it from the system
    catalogs after the fact.
    
    >> Now, what you have here is a much broader reworking.  And that's not
    >> necessarily bad, but at the moment I'm not really seeing how it
    >> benefits us.
    >>
    > In my point, if individual object types need to have its own handler for
    > alter commands, points of the code to check permissions are also
    > distributed for each object types. It shall be a burden to maintain hooks
    > that allows modules (e.g sepgsql) to have permission checks.
    
    Well, it's always nicer if you can just put a call to some hook in one
    place instead of many.  But it's not worth sacrificing everything to
    make that happen.  I think we need to compare the value of only
    needing a hook in one place against the disruption of changing a lot
    of code that is working fine as it is.  In the case of the DROP
    commands, it seems to me that the refactoring you did came out a huge
    win, but this doesn't seem as clear to me.  Note that DROP actually
    does dispatch the actual work of dropping the object to a
    type-specific function, unlike what you're trying to do here.
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
    

15. Re: Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2011-11-27T20:14:10Z

    2011/11/21 Robert Haas <robertmhaas@gmail.com>:
    >>> Now, what you have here is a much broader reworking.  And that's not
    >>> necessarily bad, but at the moment I'm not really seeing how it
    >>> benefits us.
    >>>
    >> In my point, if individual object types need to have its own handler for
    >> alter commands, points of the code to check permissions are also
    >> distributed for each object types. It shall be a burden to maintain hooks
    >> that allows modules (e.g sepgsql) to have permission checks.
    >
    > Well, it's always nicer if you can just put a call to some hook in one
    > place instead of many.  But it's not worth sacrificing everything to
    > make that happen.  I think we need to compare the value of only
    > needing a hook in one place against the disruption of changing a lot
    > of code that is working fine as it is.  In the case of the DROP
    > commands, it seems to me that the refactoring you did came out a huge
    > win, but this doesn't seem as clear to me.  Note that DROP actually
    > does dispatch the actual work of dropping the object to a
    > type-specific function, unlike what you're trying to do here.
    >
    Yes, I agree with your opinion.
    I'm also not sure whether my refactoring efforts on ALTER commands
    will give us larger worth than its size of code changes, although we will
    be able to consolidate the point of hooks around them.
    
    If we add new properties that required by AlterObjectNamespace, as
    you suggested, it will allow to reduce number of caller of this routine
    mechanically with small changes.
    Should I try this reworking with this way?
    At least, AlterObjectNamespace already consolidate the point to check
    permissions.
    
    I initially thought it is too specific for AlterObjectNamespace, then I
    reconsidered that we may be able to apply same properties to
    ALTER RENAME TO/SET OWNER commands also, even though
    these commands have completely branched routines for each object
    types.
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    

16. Re: Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement

    Robert Haas <robertmhaas@gmail.com> — 2011-11-30T20:33:14Z

    On Sun, Nov 27, 2011 at 3:14 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    > If we add new properties that required by AlterObjectNamespace, as
    > you suggested, it will allow to reduce number of caller of this routine
    > mechanically with small changes.
    > Should I try this reworking with this way?
    
    Yeah, let's try that for starters, and then see if anything further
    suggests itself.
    
    > At least, AlterObjectNamespace already consolidate the point to check
    > permissions.
    
    Right.
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
    

17. Re: Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2011-12-01T13:25:21Z

    2011/11/30 Robert Haas <robertmhaas@gmail.com>:
    > On Sun, Nov 27, 2011 at 3:14 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    >> If we add new properties that required by AlterObjectNamespace, as
    >> you suggested, it will allow to reduce number of caller of this routine
    >> mechanically with small changes.
    >> Should I try this reworking with this way?
    >
    > Yeah, let's try that for starters, and then see if anything further
    > suggests itself.
    >
    OK. I marked this patch as "Returned with Feedback".
    I will try it again.
    In the timeframe of this commit-fest, I'll focuse on remaining my
    patches and reviewing pgsql-fdw.
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>