Hi,

A customer of ours (Enova Financial) requested the ability to describe
objects in pg_depend. The wiki contains a simplistic SQL snippet that
does the task, but only for some of the object types, and it's rather
ugly. It struck me that we could fulfill this very easily by exposing
the getObjectDescription() function at the SQL level, as in the attached
module.

I propose we include this as a builtin function.

Opinions?

--
Álvaro Herrera <alvherre@alvh.no-ip.org>

Search Discussions

  • Tom Lane at Nov 17, 2010 at 3:20 pm

    Alvaro Herrera writes:
    A customer of ours (Enova Financial) requested the ability to describe
    objects in pg_depend. The wiki contains a simplistic SQL snippet that
    does the task, but only for some of the object types, and it's rather
    ugly. It struck me that we could fulfill this very easily by exposing
    the getObjectDescription() function at the SQL level, as in the attached
    module.
    What's the point of the InvalidOid check? It seems like you're mostly
    just introducing a corner case: sometimes, but not always, the function
    will return NULL instead of failing for bad input. I think it should
    just fail always.

    regards, tom lane
  • Alvaro Herrera at Nov 17, 2010 at 3:40 pm

    Excerpts from Tom Lane's message of mié nov 17 12:20:06 -0300 2010:
    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
    A customer of ours (Enova Financial) requested the ability to describe
    objects in pg_depend. The wiki contains a simplistic SQL snippet that
    does the task, but only for some of the object types, and it's rather
    ugly. It struck me that we could fulfill this very easily by exposing
    the getObjectDescription() function at the SQL level, as in the attached
    module.
    What's the point of the InvalidOid check? It seems like you're mostly
    just introducing a corner case: sometimes, but not always, the function
    will return NULL instead of failing for bad input. I think it should
    just fail always.
    If the check is not there, the calling query will have to prevent the
    function from being called on rows having OID=0 in pg_depend. (These
    rows show up in the catalog for pinned objects). The query becomes
    either incomplete (because you don't report pinned objects) or awkward
    (because you have to insert a CASE expression to avoid calling the
    function in that case).

    I don't think it's all that necessary anyway. If the function goes in
    without that check, it will still be a huge improvement over the statu
    quo.

    --
    Álvaro Herrera <alvherre@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Tom Lane at Nov 17, 2010 at 3:47 pm

    Alvaro Herrera writes:
    Excerpts from Tom Lane's message of mié nov 17 12:20:06 -0300 2010:
    What's the point of the InvalidOid check?
    If the check is not there, the calling query will have to prevent the
    function from being called on rows having OID=0 in pg_depend. (These
    rows show up in the catalog for pinned objects).
    Hmm. It would be good to document that motivation somewhere. Also,
    for my own taste it would be better to do

    /* for "pinned" items in pg_depend, return null */
    if (!OidIsValid(catalogId))
    PG_RETURN_NULL();

    ... straight line code here ...

    rather than leave the reader wondering whether there are any other cases
    where the function is intended to return null.

    Oh, one other gripe: probably better to name it pg_describe_object.

    regards, tom lane
  • Alvaro Herrera at Nov 18, 2010 at 5:23 pm
    Thanks for the comments. Updated patch attached.

    In the process of looking it over again, I noticed that in an
    assert-enabled build, it's trivial to crash the server with this
    function: just pass a nonzero subobjid with an object class that doesn't
    take one. This could be fixed easily by turning the Asserts into
    elog(ERROR).

    Another problem with this function is that a lot of checks that
    currently raise errors with elog(ERROR) are now user-facing. On this
    issue one possible answer would be to do nothing; another would be to go
    over all those calls and turn them into full-fledged ereports.

    --
    Álvaro Herrera <alvherre@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Tom Lane at Nov 18, 2010 at 5:49 pm

    Alvaro Herrera writes:
    In the process of looking it over again, I noticed that in an
    assert-enabled build, it's trivial to crash the server with this
    function: just pass a nonzero subobjid with an object class that doesn't
    take one. This could be fixed easily by turning the Asserts into
    elog(ERROR).
    Another problem with this function is that a lot of checks that
    currently raise errors with elog(ERROR) are now user-facing. On this
    issue one possible answer would be to do nothing; another would be to go
    over all those calls and turn them into full-fledged ereports.
    Yeah, it would definitely be necessary to ensure that you couldn't cause
    an Assert by passing bogus input. I wouldn't bother making the errors
    into ereports though: that's adding a lot of translation burden to no
    good purpose.


    Please do not do this:

    +/* this doesn't really need to appear in any header file */
    +Datum pg_describe_object(PG_FUNCTION_ARGS);

    Put the extern declaration in a header file, don't be cute.

    This is useless, because getObjectDescription never returns null
    (or if it does, we have a whole lot of unprotected callers to fix):

    + if (!description)
    + ereport(ERROR,
    + (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
    + errmsg("invalid object specification")));

    regards, tom lane
  • Alvaro Herrera at Nov 18, 2010 at 7:00 pm
    I just noticed that this is leaking a bit of memory, because we call
    getObjectDescription (which pallocs its result) and then
    cstring_to_text which pallocs a copy. This is not a lot and it's not
    going to live much anyway, is it worrying about? I reworked it like
    this:

    {
    char *description = NULL;
    text *tdesc;

    ...

    description = getObjectDescription(&address);
    tdesc = cstring_to_text(description);
    pfree(description);

    PG_RETURN_TEXT_P(tdesc);
    }

    I notice that ruleutils.c has a convenience function string_to_text which is
    designed to avoid this problem:

    static text *
    string_to_text(char *str)
    {
    text *result;

    result = cstring_to_text(str);
    pfree(str);
    return result;
    }

    So I could just make that non-static (though I'd move it to varlena.c
    while at it) and use that instead.

    I wonder if it's worth going through some of the other callers of
    cstring_to_text and change them to use this wrapper. There are some
    that are leaking some memory, though it's a tiny amount and I'm not
    sure it's worth the bother. (But if so, why is ruleutils going the
    extra mile?)

    --
    Álvaro Herrera <alvherre@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Tom Lane at Nov 18, 2010 at 7:20 pm

    Alvaro Herrera writes:
    I just noticed that this is leaking a bit of memory, because we call
    getObjectDescription (which pallocs its result) and then
    cstring_to_text which pallocs a copy. This is not a lot and it's not
    going to live much anyway, is it worrying about?
    No. The extra string will go away at the end of the tuple cycle.
    I don't think it's a particularly good idea for this code to assume
    that getObjectDescription's result is freeable, anyway.

    regards, tom lane
  • Alvaro Herrera at Nov 18, 2010 at 7:16 pm

    Excerpts from Tom Lane's message of jue nov 18 14:49:21 -0300 2010:

    Please do not do this:

    +/* this doesn't really need to appear in any header file */
    +Datum pg_describe_object(PG_FUNCTION_ARGS);

    Put the extern declaration in a header file, don't be cute.
    Oh, I forgot to comment on this. I had initially put the declaration in
    builtins.h, but then I noticed that namespace.c contains a bunch of
    declarations -- I even copied the comment almost verbatim:

    /* These don't really need to appear in any header file */
    Datum pg_table_is_visible(PG_FUNCTION_ARGS);
    Datum pg_type_is_visible(PG_FUNCTION_ARGS);
    Datum pg_function_is_visible(PG_FUNCTION_ARGS);
    Datum pg_operator_is_visible(PG_FUNCTION_ARGS);
    Datum pg_opclass_is_visible(PG_FUNCTION_ARGS);
    Datum pg_conversion_is_visible(PG_FUNCTION_ARGS);
    Datum pg_ts_parser_is_visible(PG_FUNCTION_ARGS);
    Datum pg_ts_dict_is_visible(PG_FUNCTION_ARGS);
    Datum pg_ts_template_is_visible(PG_FUNCTION_ARGS);
    Datum pg_ts_config_is_visible(PG_FUNCTION_ARGS);
    Datum pg_my_temp_schema(PG_FUNCTION_ARGS);
    Datum pg_is_other_temp_schema(PG_FUNCTION_ARGS);


    This seems to have originated in this commit:

    commit 4ab8e69094452286a5894f1b2b237304808f4391
    Author: Tom Lane <tgl@sss.pgh.pa.us>
    Date: Fri Aug 9 16:45:16 2002 +0000

    has_table_privilege spawns scions has_database_privilege, has_function_privilege,
    has_language_privilege, has_schema_privilege to let SQL queries test
    all the new privilege types in 7.3. Also, add functions pg_table_is_visible,
    pg_type_is_visible, pg_function_is_visible, pg_operator_is_visible,
    pg_opclass_is_visible to test whether objects contained in schemas are
    visible in the current search path. Do some minor cleanup to centralize
    accesses to pg_database, as well.


    I have to say that I'm baffled as to why has_database_privilege et al
    were declared in builtins.h but pg_table_is_visible et al in dependency.c.

    --
    Álvaro Herrera <alvherre@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Tom Lane at Nov 18, 2010 at 7:22 pm

    Alvaro Herrera writes:
    I have to say that I'm baffled as to why has_database_privilege et al
    were declared in builtins.h but pg_table_is_visible et al in dependency.c.
    builtins.h is probably the right place, on the whole ... I agree that
    consistency is somewhat lacking in this area.

    regards, tom lane

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedNov 17, '10 at 1:41p
activeNov 18, '10 at 7:22p
posts10
users3
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2021 Grokbase