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>

Search Discussions

  • Robert Haas at Nov 17, 2011 at 4:21 pm

    On Tue, Nov 15, 2011 at 4:43 AM, Kohei KaiGai 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
  • Tom Lane at Nov 17, 2011 at 6:00 pm

    Robert Haas 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
  • Robert Haas at Nov 17, 2011 at 7:25 pm

    On Thu, Nov 17, 2011 at 1:00 PM, Tom Lane 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
  • Alvaro Herrera at Nov 17, 2011 at 9:26 pm

    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 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
  • Robert Haas at Nov 17, 2011 at 10:14 pm

    On Thu, Nov 17, 2011 at 4:26 PM, Alvaro Herrera 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
  • Tom Lane at Nov 17, 2011 at 10:29 pm

    Robert Haas writes:
    On Thu, Nov 17, 2011 at 4:26 PM, Alvaro Herrera
    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
  • Robert Haas at Nov 17, 2011 at 11:45 pm

    On Thu, Nov 17, 2011 at 5:29 PM, Tom Lane wrote:
    Robert Haas <robertmhaas@gmail.com> writes:
    On Thu, Nov 17, 2011 at 4:26 PM, Alvaro Herrera
    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
  • Tom Lane at Nov 18, 2011 at 12:14 am

    Robert Haas writes:
    On Thu, Nov 17, 2011 at 5:29 PM, Tom Lane 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
  • Robert Haas at Nov 18, 2011 at 2:33 am

    On Thu, Nov 17, 2011 at 11:21 AM, Robert Haas wrote:
    On Tue, Nov 15, 2011 at 4:43 AM, Kohei KaiGai 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
  • Robert Haas at Nov 18, 2011 at 3:43 am

    On Tue, Nov 15, 2011 at 4:43 AM, Kohei KaiGai 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
  • Robert Haas at Nov 18, 2011 at 4:20 am

    On Tue, Nov 15, 2011 at 4:43 AM, Kohei KaiGai 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
  • Kohei KaiGai at Nov 19, 2011 at 6:49 pm

    2011/11/18 Robert Haas <robertmhaas@gmail.com>:
    On Tue, Nov 15, 2011 at 4:43 AM, Kohei KaiGai 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>
  • Robert Haas at Nov 21, 2011 at 8:37 pm

    On Sat, Nov 19, 2011 at 1:49 PM, Kohei KaiGai 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
  • Kohei KaiGai at Nov 27, 2011 at 8:14 pm

    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>
  • Robert Haas at Nov 30, 2011 at 8:33 pm

    On Sun, Nov 27, 2011 at 3:14 PM, Kohei KaiGai 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
  • Kohei KaiGai at Dec 1, 2011 at 1:25 pm

    2011/11/30 Robert Haas <robertmhaas@gmail.com>:
    On Sun, Nov 27, 2011 at 3:14 PM, Kohei KaiGai 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>

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedNov 15, '11 at 9:51a
activeDec 1, '11 at 1:25p
posts17
users4
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2022 Grokbase