Re: [HACKERS] describe objects, as in pg_depend
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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support 0001-Add-pg_describe_object-function.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] describe objects, as in pg_depend
Alvaro Herrera alvhe...@commandprompt.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] describe objects, as in pg_depend
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 alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] describe objects, as in pg_depend
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 */ +Datumpg_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 t...@sss.pgh.pa.us Date: Fri Aug 9 16:45:16 2002 + 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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] describe objects, as in pg_depend
Alvaro Herrera alvhe...@commandprompt.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] describe objects, as in pg_depend
Alvaro Herrera alvhe...@commandprompt.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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 alvhe...@alvh.no-ip.org
#include postgres.h
#include catalog/dependency.h
#include fmgr.h
#include utils/builtins.h
#ifdef PG_MODULE_MAGIC
PG_MODULE_MAGIC;
#endif
Datum describe_object(PG_FUNCTION_ARGS);
PG_FUNCTION_INFO_V1(describe_object);
Datum
describe_object(PG_FUNCTION_ARGS)
{
Oid classid = PG_GETARG_OID(0);
Oid objid = PG_GETARG_OID(1);
int32 subobjid = PG_GETARG_INT32(2);
ObjectAddress address;
char *description = NULL;
if (classid != InvalidOid)
{
address.classId = classid;
address.objectId = objid;
address.objectSubId = subobjid;
description = getObjectDescription(address);
}
if (!description || description[0] == '\0')
PG_RETURN_NULL();
PG_RETURN_TEXT_P(cstring_to_text(description));
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] describe objects, as in pg_depend
Alvaro Herrera alvhe...@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. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] describe objects, as in pg_depend
Excerpts from Tom Lane's message of mié nov 17 12:20:06 -0300 2010: Alvaro Herrera alvhe...@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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] describe objects, as in pg_depend
Alvaro Herrera alvhe...@commandprompt.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
