The attached three patches try to consolidate code path of DROP
statement on various kind of object classes.

The first part provides a set of internal api to reference properties
of object types; as we had a discussion before.
It adds a big static array indexed by ObjectType; that holds OID of
corresponding catalog, catcache id, attribute number of name,
namespace, owner and so on. Every field of this array is accessable
via get_object_property_*() interfaces.
As a proof-of-concept, I reworked object_exists() with this array to
lookup a heaptuple by the supplied ObjectAddress.

The second part is a revised one from what I submitted in the commit-fest July.
It consolidate existing code paths corresponding to DropStmt (schema,
relations, text searches, type, domain, extension, conversion,
collation).

The third part is more reworks for other object classes (aggregate,
function, language, foreign data wrapper, foreign server, cast,
operator, trigger, rule).

I don't touch on shared database objects (database, tablespace and
role) because its behavior on deletion is not compatible with any
other object classes. So, it is unclear for me whether it is a
suitable case to be consolidated.
And, I also don't touch code corresponding to user-mapping because it
is not supported by get_object_address() yet.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

Search Discussions

  • Kohei KaiGai at Sep 12, 2011 at 7:41 am
    I updated the patches that I send a month before.

    These are rebased to the latest tree, and the part-3 portion also consolidates
    DROP OPERATOR FAMILY/CLASS routines that I forgot to rework in the
    previous patch.

    Rest of portions are not changed.

    Thanks,

    2011/8/15 Kohei KaiGai <kaigai@kaigai.gr.jp>:
    The attached three patches try to consolidate code path of DROP
    statement on various kind of object classes.

    The first part provides a set of internal api to reference properties
    of object types; as we had a discussion before.
    It adds a big static array indexed by ObjectType; that holds OID of
    corresponding catalog, catcache id, attribute number of name,
    namespace, owner and so on. Every field of this array is accessable
    via get_object_property_*() interfaces.
    As a proof-of-concept, I reworked object_exists() with this array to
    lookup a heaptuple by the supplied ObjectAddress.

    The second part is a revised one from what I submitted in the commit-fest July.
    It consolidate existing code paths corresponding to DropStmt (schema,
    relations, text searches, type, domain, extension, conversion,
    collation).

    The third part is more reworks for other object classes (aggregate,
    function, language, foreign data wrapper, foreign server, cast,
    operator, trigger, rule).

    I don't touch on shared database objects (database, tablespace and
    role) because its behavior on deletion is not compatible with any
    other object classes. So, it is unclear for me whether it is a
    suitable case to be consolidated.
    And, I also don't touch code corresponding to user-mapping because it
    is not supported by get_object_address() yet.

    Thanks,
    --
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    --
    KaiGai Kohei <kaigai@kaigai.gr.jp>
  • Dimitri Fontaine at Sep 25, 2011 at 8:46 pm
    Hi,

    Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
    2011/8/15 Kohei KaiGai <kaigai@kaigai.gr.jp>:
    The attached three patches try to consolidate code path of DROP
    statement on various kind of object classes.
    These are rebased to the latest tree, and the part-3 portion also consolidates
    DROP OPERATOR FAMILY/CLASS routines that I forgot to rework in the
    previous patch.
    I've been reviewing those patches, that are all about code refactoring.
    I like what it's doing, generalizing ad-hoc code by adding some more
    knowledge about the source tree into some data structures. Typically,
    what catcache to use for a given object's class-id.

    The patches are not in git am format nor in patch format, so I could
    only read them, I didn't install them nor compiled the code, didn't run
    the regression tests.

    Regards,
    --
    Dimitri Fontaine
    http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
  • Dimitri Fontaine at Sep 26, 2011 at 12:43 pm

    Dimitri Fontaine writes:
    The patches are not in git am format nor in patch format, so I could
    only read them, I didn't install them nor compiled the code, didn't run
    the regression tests.
    I've marked the patch as Waiting on Author and referred to my previous
    message. Thanks,

    --
    Dimitri Fontaine
    http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
  • Kohei KaiGai at Sep 28, 2011 at 3:51 pm

    2011/9/25 Dimitri Fontaine <dimitri@2ndquadrant.fr>:
    Hi,

    Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
    2011/8/15 Kohei KaiGai <kaigai@kaigai.gr.jp>:
    The attached three patches try to consolidate code path of DROP
    statement on various kind of object classes.
    These are rebased to the latest tree, and the part-3 portion also consolidates
    DROP OPERATOR FAMILY/CLASS routines that I forgot to rework in the
    previous patch.
    I've been reviewing those patches, that are all about code refactoring.
    I like what it's doing, generalizing ad-hoc code by adding some more
    knowledge about the source tree into some data structures.  Typically,
    what catcache to use for a given object's class-id.

    The patches are not in git am format nor in patch format, so I could
    only read them, I didn't install them nor compiled the code, didn't run
    the regression tests.
    Thanks for your efforts.

    I rebased the patches towards the latest git master, so I believe these
    are available to apply.

    Thanks,
    --
    KaiGai Kohei <kaigai@kaigai.gr.jp>
  • Dimitri Fontaine at Oct 2, 2011 at 8:08 pm
    Hi,

    Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
    I've been reviewing those patches, that are all about code refactoring.
    I like what it's doing, generalizing ad-hoc code by adding some more
    knowledge about the source tree into some data structures.  Typically,
    what catcache to use for a given object's class-id.
    I rebased the patches towards the latest git master, so I believe these
    are available to apply.
    Ok I needed `git apply' to apply the patches, now that I used that I can
    confirm that the 3 patches apply, compile, pass tests, and that I could
    play with them a little. I think I'm going to mark that ready for
    commiter. I don't have enough time for a more deep review but at the
    same time patch reading and testing both passed :)

    You might need to post a version that patch will be happy with, though,
    using e.g.

    git diff |filterdiff --format=context > /tmp/foo.patch

    Then diffstat reports:
    35 files changed, 932 insertions(+), 1913 deletions(-), 345 modifications(!)

    Regards,
    --
    Dimitri Fontaine
    http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
  • Robert Haas at Oct 3, 2011 at 2:05 pm

    On Sun, Oct 2, 2011 at 4:08 PM, Dimitri Fontaine wrote:
    Ok I needed `git apply' to apply the patches, now that I used that I can
    confirm that the 3 patches apply, compile, pass tests, and that I could
    play with them a little.  I think I'm going to mark that ready for
    commiter.  I don't have enough time for a more deep review but at the
    same time patch reading and testing both passed :)

    You might need to post a version that patch will be happy with, though,
    using e.g.

     git diff |filterdiff --format=context > /tmp/foo.patch

    Then diffstat reports:
     35 files changed, 932 insertions(+), 1913 deletions(-), 345 modifications(!)
    I think that new versions of patch can handle unified diffs without a
    problem, but older versions choke on them. My Mac has 2.5.8 and
    handles unidiffs no problem.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Dimitri Fontaine at Oct 3, 2011 at 2:55 pm

    Robert Haas writes:
    I think that new versions of patch can handle unified diffs without a
    problem, but older versions choke on them. My Mac has 2.5.8 and
    handles unidiffs no problem.
    Even containing git headers?

    Here's what I'm talking about here:

    src/backend/catalog/objectaddress.c | 653 ++++++++++++++++++++++++++++++-----
    src/include/catalog/objectaddress.h | 13 +
    src/include/nodes/parsenodes.h | 2 +-
    3 files changed, 575 insertions(+), 93 deletions(-)

    diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
    index 8feb601..6094146 100644
    --- a/src/backend/catalog/objectaddress.c
    +++ b/src/backend/catalog/objectaddress.c
    @@ -82,6 +82,463 @@ static ObjectAddress get_object_address_opcf(ObjectType objtype, List *objname,
    List *objargs, bool missing_ok);
    static bool object_exists(ObjectAddress address);


    Regards,
    --
    Dimitri Fontaine
    http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
  • Robert Haas at Oct 3, 2011 at 3:16 pm

    On Mon, Oct 3, 2011 at 10:54 AM, Dimitri Fontaine wrote:
    Robert Haas <robertmhaas@gmail.com> writes:
    I think that new versions of patch can handle unified diffs without a
    problem, but older versions choke on them.  My Mac has 2.5.8 and
    handles unidiffs no problem.
    Even containing git headers?
    Yeah, it just skips right over them. I've never had even a minor
    problem on that account, which is why I was surprised to see it giving
    you so much trouble.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Tom Lane at Oct 3, 2011 at 3:28 pm

    Robert Haas writes:
    On Mon, Oct 3, 2011 at 10:54 AM, Dimitri Fontaine
    wrote:
    Robert Haas <robertmhaas@gmail.com> writes:
    I think that new versions of patch can handle unified diffs without a
    problem, but older versions choke on them. My Mac has 2.5.8 and
    handles unidiffs no problem.
    Even containing git headers?
    Yeah, it just skips right over them. I've never had even a minor
    problem on that account, which is why I was surprised to see it giving
    you so much trouble.
    I haven't observed any such problems even with the rather ancient copy
    of GNU patch on my HPUX box (seems to be 2.5.4, released in 1999).
    I vaguely recall having had to replace the even older vendor-supplied
    patch because that one didn't do unidiffs ...

    regards, tom lane
  • Robert Haas at Oct 3, 2011 at 4:20 pm

    On Mon, Oct 3, 2011 at 11:28 AM, Tom Lane wrote:
    Robert Haas <robertmhaas@gmail.com> writes:
    On Mon, Oct 3, 2011 at 10:54 AM, Dimitri Fontaine
    wrote:
    Robert Haas <robertmhaas@gmail.com> writes:
    I think that new versions of patch can handle unified diffs without a
    problem, but older versions choke on them.  My Mac has 2.5.8 and
    handles unidiffs no problem.
    Even containing git headers?
    Yeah, it just skips right over them.  I've never had even a minor
    problem on that account, which is why I was surprised to see it giving
    you so much trouble.
    I haven't observed any such problems even with the rather ancient copy
    of GNU patch on my HPUX box (seems to be 2.5.4, released in 1999).
    I vaguely recall having had to replace the even older vendor-supplied
    patch because that one didn't do unidiffs ...
    I have seen unified diffs blow up when using patch on a fairly new
    HP-UX box, but I'm not sure whether I'm using an OS-supplied copy of
    patch or something someone installed along the line somewhere.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Dimitri Fontaine at Oct 3, 2011 at 3:40 pm

    Robert Haas writes:
    Yeah, it just skips right over them. I've never had even a minor
    problem on that account, which is why I was surprised to see it giving
    you so much trouble.
    Ok then, I'll try some more next time. Been trying not to spend too
    much time here… on the other hand git apply was happy to apply it…

    Sorry for the noise,

    Regards,
    --
    Dimitri Fontaine
    http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
  • Alvaro Herrera at Oct 3, 2011 at 3:31 pm

    Excerpts from Dimitri Fontaine's message of lun oct 03 11:54:36 -0300 2011:
    Robert Haas <robertmhaas@gmail.com> writes:
    I think that new versions of patch can handle unified diffs without a
    problem, but older versions choke on them. My Mac has 2.5.8 and
    handles unidiffs no problem.
    Even containing git headers?
    I remember redirecting whole emails to "patch", and it worked just fine.
    And this wasn't recently.

    --
    �lvaro Herrera <alvherre@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Robert Haas at Oct 4, 2011 at 1:28 pm

    On Wed, Sep 28, 2011 at 11:51 AM, Kohei KaiGai wrote:
    I rebased the patches towards the latest git master, so I believe these
    are available to apply.
    Reviewing away...

    I don't see why we need one struct called ObjectProperty and another
    called CatalogProperty. Just fold CatalogProperty into ObjectProperty
    and make it one big flat structure.

    The get_object_property_waddawadda functions appear to be designed
    under the assumption that each object type will be in the
    ObjectProperty structure at the offset corresponding to (int) objtype.
    Are these functions going to get called from any place that's
    performance-critical enough to justify that optimization? I'd rather
    just have these functions use linear search, so that if someone adds a
    new OBJECT_* constant and doesn't add it to the array it doesn't
    immediately break everything.

    get_object_namespace() looks like it ought to live in objectaddress.c,
    not dropcmds.c. check_object_validation() doesn't seem like a very
    good description of what that code actually does -- and that code
    looks like it's copy-and-pasted from elsewhere. Can we avoid that?
    get_relation_address() is surely a copy of some other bit of code; how
    can we avoid duplication?

    Setting status to "Waiting on Author".

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Kohei KaiGai at Oct 5, 2011 at 8:47 am
    Thanks for your reviewing.

    2011/10/4 Robert Haas <robertmhaas@gmail.com>:
    On Wed, Sep 28, 2011 at 11:51 AM, Kohei KaiGai wrote:
    I rebased the patches towards the latest git master, so I believe these
    are available to apply.
    Reviewing away...

    I don't see why we need one struct called ObjectProperty and another
    called CatalogProperty.  Just fold CatalogProperty into ObjectProperty
    and make it one big flat structure.
    The reason of this separation is some of ObjectType shares same catalogs;
    such as pg_class for OBJECT_(TABLE|VIEW|INDEX|SEQUENCE),
    so I thought it is better to reference CatalogProperty by several ObjectProperty
    entries to avoid accidental inconsistence.
    However, it is just a matter of my preference. I'll modify it.
    The big flat structure provides a simple structure on the other hand.
    The get_object_property_waddawadda functions appear to be designed
    under the assumption that each object type will be in the
    ObjectProperty structure at the offset corresponding to (int) objtype.
     Are these functions going to get called from any place that's
    performance-critical enough to justify that optimization?  I'd rather
    just have these functions use linear search, so that if someone adds a
    new OBJECT_* constant and doesn't add it to the array it doesn't
    immediately break everything.
    OK, I'll fix it.
    I assume these functions are used in DDL operations; mostly not
    a performance critical path.
    get_object_namespace() looks like it ought to live in objectaddress.c,
    not dropcmds.c.
    OK, I'll move it.
    check_object_validation() doesn't seem like a very
    good description of what that code actually does -- and that code
    looks like it's copy-and-pasted from elsewhere.  Can we avoid that?
    I noticed that get_object_address() already applied same checks on
    pg_type object, and pg_class objects also.
    Due to the below reason, it does not invoke get_object_address() for
    the pg_class objects, so it seems to me reasonable to move whole
    of the logic in check_object_validation() to get_relation_address().
    get_relation_address() is surely a copy of some other bit of code; how
    can we avoid duplication?
    The get_relation_address() follows the logic in RemoveRelations() to be
    eliminated by this patch, so it is not a code duplication.
    The reason why we didn't consolidate this routine with get_object_address()
    was that remove-index requires locks on the table which owns the index to
    be removed, and it was ugly to add an ad-hoc if-block on the routine.

    I'll try to work on this. Please wait for a while...

    Thanks,
    --
    KaiGai Kohei <kaigai@kaigai.gr.jp>
  • Robert Haas at Oct 5, 2011 at 2:07 pm

    On Wed, Oct 5, 2011 at 4:47 AM, Kohei KaiGai wrote:
    The get_relation_address() follows the logic in RemoveRelations() to be
    eliminated by this patch, so it is not a code duplication.
    The reason why we didn't consolidate this routine with get_object_address()
    was that remove-index requires locks on the table which owns the index to
    be removed, and it was ugly to add an ad-hoc if-block on the routine.
    Yeah, that's a problem that's been in the back of my mind for a bit
    now, but I haven't come up with a good solution. I don't think
    RemoveRelations() is the only place we have this problem, either.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Kohei KaiGai at Oct 5, 2011 at 4:16 pm
    The attached patches are revised ones.

    * The array of ObjectProperty became a large flat data structure.
    * get_object_property_xxx() put off the assumption the array of
    ObjectProperty is indexed by OBJECT_* begin from 0.
    * get_object_namespace() was relocated to objectaddress.c from
    the dropcmds.c
    * The logic of check_object_validation() got included within
    get_relation_address(), and rewritten more smartly, as:

    + relkind = RelationGetForm(relation)->relkind;
    + if ((objtype == OBJECT_INDEX && relkind != RELKIND_INDEX) ||
    + (objtype == OBJECT_SEQUENCE && relkind != RELKIND_SEQUENCE) ||
    + (objtype == OBJECT_TABLE && relkind != RELKIND_RELATION) ||
    + (objtype == OBJECT_VIEW && relkind != RELKIND_VIEW) ||
    + (objtype == OBJECT_FOREIGN_TABLE && relkind != RELKIND_FOREIGN_TABLE))
    + ereport(ERROR,
    + (errcode(ERRCODE_WRONG_OBJECT_TYPE),
    + errmsg("\"%s\" is not a %s",
    + NameListToString(objname),
    + get_object_property_typetext(objtype))));
    +

    Thanks,

    2011/10/5 Robert Haas <robertmhaas@gmail.com>:
    On Wed, Oct 5, 2011 at 4:47 AM, Kohei KaiGai wrote:
    The get_relation_address() follows the logic in RemoveRelations() to be
    eliminated by this patch, so it is not a code duplication.
    The reason why we didn't consolidate this routine with get_object_address()
    was that remove-index requires locks on the table which owns the index to
    be removed, and it was ugly to add an ad-hoc if-block on the routine.
    Yeah, that's a problem that's been in the back of my mind for a bit
    now, but I haven't come up with a good solution.  I don't think
    RemoveRelations() is the only place we have this problem, either.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company


    --
    KaiGai Kohei <kaigai@kaigai.gr.jp>
  • Robert Haas at Oct 5, 2011 at 6:09 pm

    On Wed, Oct 5, 2011 at 12:16 PM, Kohei KaiGai wrote:
    * The logic of check_object_validation() got included within
     get_relation_address(), and rewritten more smartly, as:

    +   relkind = RelationGetForm(relation)->relkind;
    +   if ((objtype == OBJECT_INDEX         && relkind != RELKIND_INDEX) ||
    +       (objtype == OBJECT_SEQUENCE      && relkind != RELKIND_SEQUENCE) ||
    +       (objtype == OBJECT_TABLE         && relkind != RELKIND_RELATION) ||
    +       (objtype == OBJECT_VIEW          && relkind != RELKIND_VIEW) ||
    +       (objtype == OBJECT_FOREIGN_TABLE && relkind != RELKIND_FOREIGN_TABLE))
    +       ereport(ERROR,
    +               (errcode(ERRCODE_WRONG_OBJECT_TYPE),
    +                errmsg("\"%s\" is not a %s",
    +                       NameListToString(objname),
    +                       get_object_property_typetext(objtype))));
    +
    That's no good. We've discussed it before. It breaks translatability.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Kohei KaiGai at Oct 5, 2011 at 6:58 pm

    2011/10/5 Robert Haas <robertmhaas@gmail.com>:
    On Wed, Oct 5, 2011 at 12:16 PM, Kohei KaiGai wrote:
    * The logic of check_object_validation() got included within
     get_relation_address(), and rewritten more smartly, as:

    +   relkind = RelationGetForm(relation)->relkind;
    +   if ((objtype == OBJECT_INDEX         && relkind != RELKIND_INDEX) ||
    +       (objtype == OBJECT_SEQUENCE      && relkind != RELKIND_SEQUENCE) ||
    +       (objtype == OBJECT_TABLE         && relkind != RELKIND_RELATION) ||
    +       (objtype == OBJECT_VIEW          && relkind != RELKIND_VIEW) ||
    +       (objtype == OBJECT_FOREIGN_TABLE && relkind != RELKIND_FOREIGN_TABLE))
    +       ereport(ERROR,
    +               (errcode(ERRCODE_WRONG_OBJECT_TYPE),
    +                errmsg("\"%s\" is not a %s",
    +                       NameListToString(objname),
    +                       get_object_property_typetext(objtype))));
    +
    That's no good.  We've discussed it before.  It breaks translatability.
    Hmm. It indeed makes translation hard.
    I reverted this portion of the part-2 patch, as attached.
    Please review the newer one, instead of the previous revision.

    Thanks,
    --
    KaiGai Kohei <kaigai@kaigai.gr.jp>
  • Robert Haas at Oct 10, 2011 at 12:21 pm

    On Wed, Oct 5, 2011 at 2:58 PM, Kohei KaiGai wrote:
    Hmm. It indeed makes translation hard.
    I reverted this portion of the part-2 patch, as attached.
    Please review the newer one, instead of the previous revision.
    Please fix the compiler warnings.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Kohei KaiGai at Oct 10, 2011 at 1:39 pm

    2011/10/10 Robert Haas <robertmhaas@gmail.com>:
    On Wed, Oct 5, 2011 at 2:58 PM, Kohei KaiGai wrote:
    Hmm. It indeed makes translation hard.
    I reverted this portion of the part-2 patch, as attached.
    Please review the newer one, instead of the previous revision.
    Please fix the compiler warnings.
    I checked compiler warnings using COPT=-Werror, but it detects warning on
    only unrelated files as below. Does it really come from my patches?
    (Does it depend on ./configure options?)

    gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith
    -Wdeclaration-after-statement -Wendif-labels
    -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
    -fwrapv -g -Werror -I../../../src/include -D_GNU_SOURCE -c -o
    execQual.o execQual.c
    execQual.c: In function ‘GetAttributeByNum’:
    execQual.c:1104:67: error: the comparison will always evaluate as
    ‘true’ for the address of ‘tmptup’ will never be NULL
    [-Werror=address]
    execQual.c: In function ‘GetAttributeByName’:
    execQual.c:1165:67: error: the comparison will always evaluate as
    ‘true’ for the address of ‘tmptup’ will never be NULL
    [-Werror=address]
    execQual.c: In function ‘ExecEvalFieldSelect’:
    execQual.c:3914:67: error: the comparison will always evaluate as
    ‘true’ for the address of ‘tmptup’ will never be NULL
    [-Werror=address]
    cc1: all warnings being treated as errors


    gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith
    -Wdeclaration-after-statement -Wendif-labels
    -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
    -fwrapv -g -Werror -I../../../../src/include -D_GNU_SOURCE -c -o
    tuplesort.o tuplesort.c
    tuplesort.c: In function ‘comparetup_heap’:
    tuplesort.c:2751:66: error: the comparison will always evaluate as
    ‘true’ for the address of ‘ltup’ will never be NULL [-Werror=address]
    tuplesort.c:2752:66: error: the comparison will always evaluate as
    ‘true’ for the address of ‘rtup’ will never be NULL [-Werror=address]
    tuplesort.c: In function ‘copytup_heap’:
    tuplesort.c:2783:71: error: the comparison will always evaluate as
    ‘true’ for the address of ‘htup’ will never be NULL [-Werror=address]
    tuplesort.c: In function ‘readtup_heap’:
    tuplesort.c:2835:71: error: the comparison will always evaluate as
    ‘true’ for the address of ‘htup’ will never be NULL [-Werror=address]
    cc1: all warnings being treated as errors

    Thanks,
    --
    KaiGai Kohei <kaigai@kaigai.gr.jp>
  • Robert Haas at Oct 10, 2011 at 2:26 pm

    On Mon, Oct 10, 2011 at 9:39 AM, Kohei KaiGai wrote:
    2011/10/10 Robert Haas <robertmhaas@gmail.com>:
    On Wed, Oct 5, 2011 at 2:58 PM, Kohei KaiGai wrote:
    Hmm. It indeed makes translation hard.
    I reverted this portion of the part-2 patch, as attached.
    Please review the newer one, instead of the previous revision.
    Please fix the compiler warnings.
    I checked compiler warnings using COPT=-Werror, but it detects warning on
    only unrelated files as below. Does it really come from my patches?
    (Does it depend on ./configure options?)
    OK, well, I applied pgsql-v9.2-drop-reworks-2.v4.1.patch and tried to
    compile, and got this:

    In file included from ../../../src/include/catalog/dependency.h:17,
    from dependency.c:19:
    ../../../src/include/catalog/objectaddress.h:21: warning: type
    defaults to ‘int’ in declaration of ‘ObjectAddress’
    ../../../src/include/catalog/objectaddress.h:21: error: expected ‘;’,
    ‘,’ or ‘)’ before ‘*’ token

    The problem here is pretty obvious: you've defined
    get_object_namespace, which takes an argument of type ObjectAddress,
    before defining the ObjectAddress datatype, which is the next thing in
    the same header file. How does that even compile for you?

    That's easy enough to fix, but then I get this:

    objectaddress.c: In function ‘get_object_namespace’:
    objectaddress.c:996: warning: implicit declaration of function
    ‘get_object_property_attnum_namespace’
    objectaddress.c:1000: warning: implicit declaration of function
    ‘get_object_property_catid_by_oid’
    objectaddress.c:1006: warning: implicit declaration of function
    ‘get_object_property_typetext’
    objectaddress.c:1006: warning: format ‘%s’ expects type ‘char *’, but
    argument 3 has type ‘int’

    Maybe the problem here is that I've only applied the first patch, and
    this stuff is cleaned up in the later patches in the series. But what
    is the point of separating out the patches if you can't even compile
    with only some of them applied?

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Kohei KaiGai at Oct 10, 2011 at 6:09 pm

    OK, well, I applied pgsql-v9.2-drop-reworks-2.v4.1.patch and tried to
    compile, and got this:

    In file included from ../../../src/include/catalog/dependency.h:17,
    from dependency.c:19:
    ../../../src/include/catalog/objectaddress.h:21: warning: type
    defaults to ‘int’ in declaration of ‘ObjectAddress’
    ../../../src/include/catalog/objectaddress.h:21: error: expected ‘;’,
    ‘,’ or ‘)’ before ‘*’ token

    The problem here is pretty obvious: you've defined
    get_object_namespace, which takes an argument of type ObjectAddress,
    before defining the ObjectAddress datatype, which is the next thing in
    the same header file.  How does that even compile for you?
    Sorry, I didn't write out dependency of the patches.
    Please apply the patches in order of part-1, part-2 then part-3.

    I checked correctness of the part-2 on the tree with the part-1 already.
    Both of the part-1 and part-2 patches try to modify objectaddress.h,
    and the part-2 tries to add get_object_namespace() definition around
    the code added by the part-1, so the patch commands get confused
    and moved the hunk in front of the definition of ObjectAddress.

    [kaigai@iwashi pgsql]$ cat
    ~/patch/pgsql-v9.2-drop-reworks-2.v4.1.patch | patch -p1
    :
    patching file src/backend/catalog/objectaddress.c
    Hunk #1 succeeded at 976 (offset -429 lines).
    :
    patching file src/include/catalog/objectaddress.h
    Hunk #1 succeeded at 17 with fuzz 2 (offset -18 lines).
    :
    patching file src/test/regress/expected/drop_if_exists.out

    I'm sorry again. I tought it was obvious from the filenames.

    * Part-1
    pgsql-v9.2-drop-reworks-1.v4.patch
    * Part-2 (depends on Part-1)
    pgsql-v9.2-drop-reworks-2.v4.1.patch
    * Part-3 (depends on Part-1 and -2)
    pgsql-v9.2-drop-reworks-3.v4.patch

    Thanks,
    --
    KaiGai Kohei <kaigai@kaigai.gr.jp>
  • Robert Haas at Oct 11, 2011 at 1:44 pm

    On Mon, Oct 10, 2011 at 1:38 PM, Kohei KaiGai wrote:
    I'm sorry again. I tought it was obvious from the filenames.
    I guess I got confused because you re-posted part 2 without the other
    parts, and I got mixed up and thought you were reposting part one.

    I've committed a stripped-down version of the part one patch, which
    had several mistakes even in just the part I committed - e.g., you
    forgot TABLESPACEOID. I also did some renaming for clarity.

    I'm going to throw this back to you for rebasing at this point, which
    I realize is going to be somewhat of an unenjoyable task given the way
    I cut up your changes to objectaddress.c, but I wasn't very confident
    that all of the entries were correct (the one for attributes seemed
    clearly wrong to me, for example), and I didn't want to commit a bunch
    of stuff that wasn't going to be exercised. I suggest that you merge
    the remainder of the part-one changes into part-two. On the flip
    side, I think you should take the stuff that deals with dropping
    relations OUT of part two. I don't see what good it does us to try to
    centralize the drop logic if we still have to have special cases for
    relations, so let's just leave that separate for now until we figure
    out a better approach, or at least split it off as a separate patch so
    that it doesn't hold up all the other changes.

    I think get_object_namespace() needs substantial revision. Instead of
    passing the object type and the object address, why not just pass the
    object address? You should be able to use the classId in the address
    to figure out everything you need to know. Since this function is
    private to objectaddress.c, there's no reason for it to use those
    accessor functions - it can just iterate through the array just as
    object_exists() does. That way you also avoid iterating through the
    array multiple times. I also think that we probably ought to revise
    AlterObjectNamespace() to make use of this new machinery, instead of
    making the caller pass in all the same information that
    objectaddress.c is now learning how to provide. That would possibly
    open the way to a bunch more consolidation of the SET SCHEMA code; in
    fact, we might want to clean that up first, before dealing with the
    DROP stuff.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Kohei KaiGai at Oct 12, 2011 at 12:08 pm
    Robert,

    I'm currently trying to revise my patches according to your suggestions,
    but I'm facing a trouble about error messages when user tries to drop
    a non-exists object.

    Because the ObjectProperty array has an entry for each catalogs, it is
    unavailable to hold the name of object type (such as "table" or "index")
    when multiple object types are associated with a particular system
    catalog, such as pg_class, pg_type or pg_proc.

    How should I implement the following block?

    if (!OidIsValid(address.objectId))
    {
    ereport(NOTICE,
    (errmsg("%s \"%s\" does not exist, skipping",
    get_object_property_typetext(stmt->removeType),
    NameListToString(objname))));
    continue;
    }

    One idea is to add a separated array to translate from OBJECT_* to
    its text representation. (Maybe, it can be used to translattions with
    opposite direction.)

    Thanks,

    2011/10/11 Robert Haas <robertmhaas@gmail.com>:
    On Mon, Oct 10, 2011 at 1:38 PM, Kohei KaiGai wrote:
    I'm sorry again. I tought it was obvious from the filenames.
    I guess I got confused because you re-posted part 2 without the other
    parts, and I got mixed up and thought you were reposting part one.

    I've committed a stripped-down version of the part one patch, which
    had several mistakes even in just the part I committed - e.g., you
    forgot TABLESPACEOID.  I also did some renaming for clarity.

    I'm going to throw this back to you for rebasing at this point, which
    I realize is going to be somewhat of an unenjoyable task given the way
    I cut up your changes to objectaddress.c, but I wasn't very confident
    that all of the entries were correct (the one for attributes seemed
    clearly wrong to me, for example), and I didn't want to commit a bunch
    of stuff that wasn't going to be exercised.  I suggest that you merge
    the remainder of the part-one changes into part-two.  On the flip
    side, I think you should take the stuff that deals with dropping
    relations OUT of part two.  I don't see what good it does us to try to
    centralize the drop logic if we still have to have special cases for
    relations, so let's just leave that separate for now until we figure
    out a better approach, or at least split it off as a separate patch so
    that it doesn't hold up all the other changes.

    I think get_object_namespace() needs substantial revision.  Instead of
    passing the object type and the object address, why not just pass the
    object address?  You should be able to use the classId in the address
    to figure out everything you need to know.  Since this function is
    private to objectaddress.c, there's no reason for it to use those
    accessor functions - it can just iterate through the array just as
    object_exists() does.  That way you also avoid iterating through the
    array multiple times.  I also think that we probably ought to revise
    AlterObjectNamespace() to make use of this new machinery, instead of
    making the caller pass in all the same information that
    objectaddress.c is now learning how to provide.  That would possibly
    open the way to a bunch more consolidation of the SET SCHEMA code; in
    fact, we might want to clean that up first, before dealing with the
    DROP stuff.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company


    --
    KaiGai Kohei <kaigai@kaigai.gr.jp>
  • Robert Haas at Oct 12, 2011 at 12:46 pm

    On Wed, Oct 12, 2011 at 8:07 AM, Kohei KaiGai wrote:
    I'm currently trying to revise my patches according to your suggestions,
    but I'm facing a trouble about error messages when user tries to drop
    a non-exists object.

    Because the ObjectProperty array has an entry for each catalogs, it is
    unavailable to hold the name of object type (such as "table" or "index")
    when multiple object types are associated with a particular system
    catalog, such as pg_class, pg_type or pg_proc.

    How should I implement the following block?

           if (!OidIsValid(address.objectId))
           {
               ereport(NOTICE,
                       (errmsg("%s \"%s\" does not exist, skipping",
                               get_object_property_typetext(stmt->removeType),
                               NameListToString(objname))));
               continue;
           }

    One idea is to add a separated array to translate from OBJECT_* to
    its text representation. (Maybe, it can be used to translattions with
    opposite direction.)
    For reasons of translation, you can't do something like "%s \"%s\"
    does not exist, skipping". Instead I think you need an array that
    works something like dropmsgstringarray[], but based on the OBJECT_*
    constants rather than the RELKIND_* constants. IOW, it maps the
    object type to the full error message, not just the name of the object
    type.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Kohei KaiGai at Oct 12, 2011 at 12:53 pm

    2011/10/12 Robert Haas <robertmhaas@gmail.com>:
    On Wed, Oct 12, 2011 at 8:07 AM, Kohei KaiGai wrote:
    I'm currently trying to revise my patches according to your suggestions,
    but I'm facing a trouble about error messages when user tries to drop
    a non-exists object.

    Because the ObjectProperty array has an entry for each catalogs, it is
    unavailable to hold the name of object type (such as "table" or "index")
    when multiple object types are associated with a particular system
    catalog, such as pg_class, pg_type or pg_proc.

    How should I implement the following block?

           if (!OidIsValid(address.objectId))
           {
               ereport(NOTICE,
                       (errmsg("%s \"%s\" does not exist, skipping",
                               get_object_property_typetext(stmt->removeType),
                               NameListToString(objname))));
               continue;
           }

    One idea is to add a separated array to translate from OBJECT_* to
    its text representation. (Maybe, it can be used to translattions with
    opposite direction.)
    For reasons of translation, you can't do something like "%s \"%s\"
    does not exist, skipping".  Instead I think you need an array that
    works something like dropmsgstringarray[], but based on the OBJECT_*
    constants rather than the RELKIND_* constants.  IOW, it maps the
    object type to the full error message, not just the name of the object
    type.
    OK, I'll revise the code based on this idea.

    Thanks,
    --
    KaiGai Kohei <kaigai@kaigai.gr.jp>
  • Kohei KaiGai at Oct 13, 2011 at 4:46 pm
    The attached patches are revised one, based on the latest version.

    The get_object_namespace() got simplified to take only ObjectAddress,
    and attnum_namespace was added to ObjectPropertyType array.

    The DropErrorMsgNonExistent() is a function to raise a notice
    when the supplied object name was missing and IF EXISTS was
    also appended.
    Unfortunately, it was not possible to hold all the notification
    messages for each object types an array something like
    dropmsgstringarray[] in tablecmds.c, because some of object
    type requires to generate "%s" part using characteristic way.

    Like:
    + case OBJECT_CAST:
    + msgfmt = gettext_noop("cast from type %s to type %s does
    not exist, skipping");
    + marg1 = format_type_be(typenameTypeId(NULL,
    + (TypeName *) linitial(objname)));
    + marg2 = format_type_be(typenameTypeId(NULL,
    + (TypeName *) linitial(objargs)));
    + break;

    So, I used a big switch statement to handle notification messages
    when IF EXISTS was given.

    And, also I added regression test cases to detect these code paths,
    because some of object types does not cover the case when it was
    dropped.

    Reworking ALTER xxx SET SCHEMA/RENAME TO/SET OWNER
    may be a homework of mine towards the next commit fest?

    Thanks,

    2011/10/11 Robert Haas <robertmhaas@gmail.com>:
    On Mon, Oct 10, 2011 at 1:38 PM, Kohei KaiGai wrote:
    I'm sorry again. I tought it was obvious from the filenames.
    I guess I got confused because you re-posted part 2 without the other
    parts, and I got mixed up and thought you were reposting part one.

    I've committed a stripped-down version of the part one patch, which
    had several mistakes even in just the part I committed - e.g., you
    forgot TABLESPACEOID.  I also did some renaming for clarity.

    I'm going to throw this back to you for rebasing at this point, which
    I realize is going to be somewhat of an unenjoyable task given the way
    I cut up your changes to objectaddress.c, but I wasn't very confident
    that all of the entries were correct (the one for attributes seemed
    clearly wrong to me, for example), and I didn't want to commit a bunch
    of stuff that wasn't going to be exercised.  I suggest that you merge
    the remainder of the part-one changes into part-two.  On the flip
    side, I think you should take the stuff that deals with dropping
    relations OUT of part two.  I don't see what good it does us to try to
    centralize the drop logic if we still have to have special cases for
    relations, so let's just leave that separate for now until we figure
    out a better approach, or at least split it off as a separate patch so
    that it doesn't hold up all the other changes.

    I think get_object_namespace() needs substantial revision.  Instead of
    passing the object type and the object address, why not just pass the
    object address?  You should be able to use the classId in the address
    to figure out everything you need to know.  Since this function is
    private to objectaddress.c, there's no reason for it to use those
    accessor functions - it can just iterate through the array just as
    object_exists() does.  That way you also avoid iterating through the
    array multiple times.  I also think that we probably ought to revise
    AlterObjectNamespace() to make use of this new machinery, instead of
    making the caller pass in all the same information that
    objectaddress.c is now learning how to provide.  That would possibly
    open the way to a bunch more consolidation of the SET SCHEMA code; in
    fact, we might want to clean that up first, before dealing with the
    DROP stuff.
    --
    KaiGai Kohei <kaigai@kaigai.gr.jp>
  • Robert Haas at Oct 19, 2011 at 4:01 pm

    On Thu, Oct 13, 2011 at 12:46 PM, Kohei KaiGai wrote:
    And, also I added regression test cases to detect these code paths,
    because some of object types does not cover the case when it was
    dropped.
    These regression tests seem busted to me. First, I applied the part 2
    patch. The regression tests failed. Then, I applied the part 3
    patch. Then they passed. So far so good. Then, I took the
    regression test portion of the part 2 and part 3 patches and applied
    just those. That also fails.

    Can we come up with a set of regression tests that:

    - passes on unmodified master
    - still passes with the part 2 patch applied
    - also passes with both the part 2 and part 3 patches applied

    AIUI, this patch isn't supposed to be changing any behavior, just
    consolidating the code.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Kohei KaiGai at Oct 19, 2011 at 7:30 pm

    2011/10/19 Robert Haas <robertmhaas@gmail.com>:
    On Thu, Oct 13, 2011 at 12:46 PM, Kohei KaiGai wrote:
    And, also I added regression test cases to detect these code paths,
    because some of object types does not cover the case when it was
    dropped.
    These regression tests seem busted to me.  First, I applied the part 2
    patch.  The regression tests failed.  Then, I applied the part 3
    patch.  Then they passed.  So far so good.  Then, I took the
    regression test portion of the part 2 and part 3 patches and applied
    just those.  That also fails.
    Sorry, it was my misses when I added regression test cases.
    Can we come up with a set of regression tests that:

    - passes on unmodified master
    - still passes with the part 2 patch applied
    - also passes with both the part 2 and part 3 patches applied

    AIUI, this patch isn't supposed to be changing any behavior, just
    consolidating the code.
    The attached patches are:

    part-1: only regression test of DROP [IF EXISTS] on unmodified master
    part-2: drop statement reworks that uses T_DropStmt
    part-3: drop statement reworks for other object classes

    Unfortunately, the part-3 had to change regression test portion a bit,
    because ...
    - Unmodified master does not print ", skipping" when we tried to
    drop non-existence operator class with IF EXISTS.
    - Unmodified master raised an error, not notice, when we tried to
    drop non-existence operator family with IF EXISTS.
    Is it a bug to be fixed, isn't it?

    Thanks,
    --
    KaiGai Kohei <kaigai@kaigai.gr.jp>
  • Robert Haas at Oct 20, 2011 at 3:33 am

    On Wed, Oct 19, 2011 at 3:30 PM, Kohei KaiGai wrote:
    part-1: only regression test of DROP [IF EXISTS] on unmodified master
    Committed. Which I just noticed broke the build farm. Will go fix that now...
    part-2: drop statement reworks that uses T_DropStmt
    Committed with some changes.
    part-3: drop statement reworks for other object classes
    This is going to need some rebasing.
    Unfortunately, the part-3 had to change regression test portion a bit,
    because ...
     - Unmodified master does not print ", skipping" when we tried to
      drop non-existence operator class with IF EXISTS.
    OK, we should fix that.
     - Unmodified master raised an error, not notice, when we tried to
      drop non-existence operator family with IF EXISTS.
      Is it a bug to be fixed, isn't it?
    Yeah, that seems like a bug.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Robert Haas at Oct 20, 2011 at 4:57 am

    On Wed, Oct 19, 2011 at 11:33 PM, Robert Haas wrote:
    Committed.  Which I just noticed broke the build farm.  Will go fix that now...
    Wow, that was a lot of pretty colors. Or not so pretty colors.

    Seems to be turning green again now, so I'm going to bed. Will check
    it again in the morning.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Kohei KaiGai at Oct 20, 2011 at 3:14 pm

    part-3: drop statement reworks for other object classes
    This is going to need some rebasing.
    OK, I rebased it.

    This patch includes bug-fix when we tried to drop non-existence
    operator family with IF EXISTS.

    Thanks,
    --
    KaiGai Kohei <kaigai@kaigai.gr.jp>
  • Robert Haas at Oct 20, 2011 at 4:02 pm

    On Thu, Oct 20, 2011 at 10:49 AM, Kohei KaiGai wrote:
    part-3: drop statement reworks for other object classes
    This is going to need some rebasing.
    OK, I rebased it.

    This patch includes bug-fix when we tried to drop non-existence
    operator family with IF EXISTS.
    I'm thinking we should probably pull that part out and do it
    separately. Seems like it should probably be back-patched.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Kohei KaiGai at Oct 21, 2011 at 9:08 am

    2011/10/20 Robert Haas <robertmhaas@gmail.com>:
    On Thu, Oct 20, 2011 at 10:49 AM, Kohei KaiGai wrote:
    part-3: drop statement reworks for other object classes
    This is going to need some rebasing.
    OK, I rebased it.

    This patch includes bug-fix when we tried to drop non-existence
    operator family with IF EXISTS.
    I'm thinking we should probably pull that part out and do it
    separately.  Seems like it should probably be back-patched.
    I checked REL9_0_STABLE branch:

    It seems to me v9.0 implementation is correct. It might be enbugged
    when OpFamilyCacheLookup() get missing_ok argument. :-(

    /*
    * RemoveOpFamily
    * Deletes an opfamily.
    */
    void
    RemoveOpFamily(RemoveOpFamilyStmt *stmt)
    {
    Oid amID,
    opfID;
    HeapTuple tuple;
    ObjectAddress object;

    /*
    * Get the access method's OID.
    */
    amID = GetSysCacheOid1(AMNAME, CStringGetDatum(stmt->amname));
    if (!OidIsValid(amID))
    ereport(ERROR,
    (errcode(ERRCODE_UNDEFINED_OBJECT),
    errmsg("access method \"%s\" does not exist",
    stmt->amname)));

    /*
    * Look up the opfamily.
    */
    tuple = OpFamilyCacheLookup(amID, stmt->opfamilyname);
    if (!HeapTupleIsValid(tuple))
    {
    if (!stmt->missing_ok)
    ereport(ERROR,
    (errcode(ERRCODE_UNDEFINED_OBJECT),
    errmsg("operator family \"%s\" does not exist for
    access method \"%s\"",
    NameListToString(stmt->opfamilyname), stmt->amname)));
    else
    ereport(NOTICE,
    (errmsg("operator family \"%s\" does not exist for
    access method \"%s\"",
    NameListToString(stmt->opfamilyname), stmt->amname)));
    return;
    }

    --
    KaiGai Kohei <kaigai@kaigai.gr.jp>
  • Robert Haas at Oct 21, 2011 at 1:02 pm

    On Fri, Oct 21, 2011 at 5:08 AM, Kohei KaiGai wrote:
    It seems to me v9.0 implementation is correct. It might be enbugged
    when OpFamilyCacheLookup() get missing_ok argument. :-(
    Yep, looks that way. Will fix.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedAug 15, '11 at 8:54p
activeOct 21, '11 at 1:02p
posts36
users5
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2022 Grokbase