describe objects, as in pg_depend

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(at)alvh(dot)no-ip(dot)org>

Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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.

regards, tom lane

Excerpts from Tom Lane's message of mié nov 17 12:20:06 -0300 2010:
> Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> 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

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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> 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

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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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(at)sss(dot)pgh(dot)pa(dot)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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> 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 <alvherre(at)commandprompt(dot)com> 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