Re: [HACKERS] pg_typeof() patch review
On Nov 3, 2008, at 11:15 AM, Tom Lane wrote: David E. Wheeler [EMAIL PROTECTED] writes: On Nov 3, 2008, at 10:52 AM, Alvaro Herrera wrote: Maybe we should link to this page in the pg_typeof() description. Also, perhaps this page needs more examples. Yes, both of those would help a lot, I think. Feel free to send in a docs patch ... Well, I wasn't sure of the appropriate place to add examples to datatype.sgml. But this patch would certainly make the output of pg_typeof() much clearer to newbies like me. Thanks, David pg_typeof_doc.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] pg_typeof() patch review
David E. Wheeler [EMAIL PROTECTED] writes: On Nov 3, 2008, at 11:15 AM, Tom Lane wrote: Feel free to send in a docs patch ... Well, I wasn't sure of the appropriate place to add examples to datatype.sgml. But this patch would certainly make the output of pg_typeof() much clearer to newbies like me. Applied with some further editorialization. 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] pg_typeof() patch review
On Nov 7, 2008, at 2:55 PM, Tom Lane wrote: Well, I wasn't sure of the appropriate place to add examples to datatype.sgml. But this patch would certainly make the output of pg_typeof() much clearer to newbies like me. Applied with some further editorialization. Thanks! David -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] pg_typeof() patch review
Hi,
Brendan Jurd submitted a patch to add a pg_typeof() builtin function:
http://archives.postgresql.org/pgsql-patches/2008-09/msg00029.php
I've reviewed the patch and it looks fine. An updated version is
attached, having made these changes:
1) Rebased to current CVS head
2) func.sgml: clarifying that the function returns an OID rather
than a string
3) catversion.h: updated catalog version with today's date
4) pg_proc.h: placed the new entry in numerical order (Note: Does
it matter how new pg_proc OIDs are assigned? I assume any
available OID - 826 in this case - is as good as any other?)
5) polymorphism.sql/polymorphism.out: added regression test for
the new function
I hope the attached patch is formatted ok - this is how it came
from Mercurial. I applied it using patch -p 1.
This is my first review, so I welcome your feedback on whether
I'm doing it right.
Regards,
... kurt
diff -r 4b92d79506ba doc/src/sgml/func.sgml
--- a/doc/src/sgml/func.sgmlMon Nov 03 01:17:08 2008 +
+++ b/doc/src/sgml/func.sgmlMon Nov 03 00:13:50 2008 -0800
@@ -11592,6 +11592,10 @@
/indexterm
indexterm
+primarypg_typeof/primary
+ /indexterm
+
+ indexterm
primarypg_get_keywords/primary
/indexterm
@@ -11660,6 +11664,11 @@
entryliteralfunctionformat_type/function(parametertype_oid/parameter,
parametertypemod/)/literal/entry
entrytypetext/type/entry
entryget SQL name of a data type/entry
+ /row
+ row
+
entryliteralfunctionpg_typeof/function(parameterany/parameter)/literal/entry
+ entrytyperegtype/type/entry
+ entryget the data type of any value/entry
/row
row
entryliteralfunctionpg_get_keywords/function()/literal/entry
@@ -11774,6 +11783,12 @@
functionformat_type/function returns the SQL name of a data type that
is identified by its type OID and possibly a type modifier. Pass NULL
for the type modifier if no specific modifier is known.
+ /para
+
+ para
+ functionpg_typeof/function returns the OID of the data type of any
+ value which is passed to it as an argument. This can be helpful for
+ troubleshooting or dynamically constructing SQL queries.
/para
para
diff -r 4b92d79506ba src/backend/utils/adt/misc.c
--- a/src/backend/utils/adt/misc.c Mon Nov 03 01:17:08 2008 +
+++ b/src/backend/utils/adt/misc.c Mon Nov 03 00:13:51 2008 -0800
@@ -35,6 +35,15 @@
#define atooid(x) ((Oid) strtoul((x), NULL, 10))
+
+/*
+ * Return the type of the argument.
+ */
+Datum
+pg_typeof(PG_FUNCTION_ARGS)
+{
+ PG_RETURN_OID(get_fn_expr_argtype(fcinfo-flinfo, 0));
+}
/*
* current_database()
diff -r 4b92d79506ba src/include/catalog/catversion.h
--- a/src/include/catalog/catversion.h Mon Nov 03 01:17:08 2008 +
+++ b/src/include/catalog/catversion.h Mon Nov 03 00:13:51 2008 -0800
@@ -53,6 +53,6 @@
*/
/* mmddN */
-#define CATALOG_VERSION_NO 200810311
+#define CATALOG_VERSION_NO 200811021
#endif
diff -r 4b92d79506ba src/include/catalog/pg_proc.h
--- a/src/include/catalog/pg_proc.h Mon Nov 03 01:17:08 2008 +
+++ b/src/include/catalog/pg_proc.h Mon Nov 03 00:13:51 2008 -0800
@@ -1085,6 +1085,9 @@
DESCR(greater-than-or-equal);
/* OIDS 800 - 899 */
+
+DATA(insert OID = 826 ( pg_typeof PGNSP PGUID 12 1 0 0 f f f f
i 1 2206 2276 _null_ _null_ _null_ pg_typeof _null_ _null_ _null_ ));
+DESCR(returns the type of the argument);
DATA(insert OID = 846 ( cash_mul_flt4PGNSP PGUID 12 1 0 0 f f t f i 2
790 790 700 _null_ _null_ _null_ cash_mul_flt4 _null_ _null_ _null_ ));
DESCR(multiply);
diff -r 4b92d79506ba src/include/utils/builtins.h
--- a/src/include/utils/builtins.h Mon Nov 03 01:17:08 2008 +
+++ b/src/include/utils/builtins.h Mon Nov 03 00:13:51 2008 -0800
@@ -395,6 +395,7 @@
extern Datum pg_ls_dir(PG_FUNCTION_ARGS);
/* misc.c */
+extern Datum pg_typeof(PG_FUNCTION_ARGS);
extern Datum current_database(PG_FUNCTION_ARGS);
extern Datum current_query(PG_FUNCTION_ARGS);
extern Datum pg_cancel_backend(PG_FUNCTION_ARGS);
diff -r 4b92d79506ba src/test/regress/expected/polymorphism.out
--- a/src/test/regress/expected/polymorphism.outMon Nov 03 01:17:08
2008 +
+++ b/src/test/regress/expected/polymorphism.outMon Nov 03 00:13:51
2008 -0800
@@ -688,7 +688,6 @@
(1 row)
-drop function concat(text, anyarray);
-- mix variadic with anyelement
create function formarray(anyelement, variadic anyarray) returns anyarray as $$
select array_prepend($1, $2);
@@ -720,4 +719,52 @@
LINE 1: select formarray(1, variadic array['x'::text]);
^
HINT: No function matches the given name and argument types. You might need
to add explicit type casts.
+-- test pg_typeof() function
+select pg_typeof(null), -- unknown
+ pg_typeof(0), -- integer
+ pg_typeof(0.0),--
Re: [HACKERS] pg_typeof() patch review
On Nov 3, 2008, at 1:28 AM, Kurt Harriman wrote: 2) func.sgml: clarifying that the function returns an OID rather than a string Actually, it returns a regtype, no? Best, David -- 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] pg_typeof() patch review
David E. Wheeler escribió: On Nov 3, 2008, at 1:28 AM, Kurt Harriman wrote: 2) func.sgml: clarifying that the function returns an OID rather than a string Actually, it returns a regtype, no? Yes -- regtype, which is an OID alias type. -- Alvaro Herrerahttp://www.CommandPrompt.com/ 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] pg_typeof() patch review
David E. Wheeler [EMAIL PROTECTED] writes: On Nov 3, 2008, at 1:28 AM, Kurt Harriman wrote: 2) func.sgml: clarifying that the function returns an OID rather than a string Actually, it returns a regtype, no? I thought the description was good, because it emphasizes that the result is-a OID; the table entry says regtype but people might not realize that that means they can use it as, eg, something to compare to pg_attribute.atttypid. 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] pg_typeof() patch review
On Nov 3, 2008, at 10:02 AM, Tom Lane wrote: David E. Wheeler [EMAIL PROTECTED] writes: On Nov 3, 2008, at 1:28 AM, Kurt Harriman wrote: 2) func.sgml: clarifying that the function returns an OID rather than a string Actually, it returns a regtype, no? I thought the description was good, because it emphasizes that the result is-a OID; the table entry says regtype but people might not realize that that means they can use it as, eg, something to compare to pg_attribute.atttypid. Well, as someone who was until recently unfamiliar with regtypes, and who thinks of an OID as essentially just a number, I would find it very useful if the description indicated that, as a regtype, the return value could be used as either an OID or as string. Otherwise, I'd find the description kind of confusing (in one place it says it returns a regtype, whatever *that* is, and in one place it says an OID). Just thinking at this from the point of view of a relative newbiew… Thanks, David -- 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] pg_typeof() patch review
David E. Wheeler escribió: Well, as someone who was until recently unfamiliar with regtypes, and who thinks of an OID as essentially just a number, I would find it very useful if the description indicated that, as a regtype, the return value could be used as either an OID or as string. Otherwise, I'd find the description kind of confusing (in one place it says it returns a regtype, whatever *that* is, and in one place it says an OID). Give this a read http://www.postgresql.org/docs/8.3/static/datatype-oid.html Maybe we should link to this page in the pg_typeof() description. Also, perhaps this page needs more examples. -- Alvaro Herrerahttp://www.CommandPrompt.com/ 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] pg_typeof() patch review
Kurt Harriman [EMAIL PROTECTED] writes: Brendan Jurd submitted a patch to add a pg_typeof() builtin function: http://archives.postgresql.org/pgsql-patches/2008-09/msg00029.php I've reviewed the patch and it looks fine. An updated version is attached, having made these changes: Applied, thanks. 3) catversion.h: updated catalog version with today's date Actually, best practice is simply to remind the committer in the text of the message that a catversion bump is required. Including that in the patch isn't very helpful for two reasons: it's unlikely to be the right date by the time the patch is applied, and it's *extremely* likely to result in a merge failure due to some other patch having gone in first. 4) pg_proc.h: placed the new entry in numerical order (Note: Does it matter how new pg_proc OIDs are assigned? I assume any available OID - 826 in this case - is as good as any other?) I put it beside pg_get_keywords since that was where it was in the docs and source code, and chose a free OID as close as I could get to that. There's not any real solid policy about manual OID assignment. In this case the only consideration I can think of is to try to avoid creating a merge conflict with other pending patches. Best chance at that (if you only need one OID) is to make sure you've sucked up a lone OID rather than a member of a block of free OIDs. 5) polymorphism.sql/polymorphism.out: added regression test for the new function I thought the test was overkill for a one-liner function, and simplified it a bit. I agree that no test at all might have been too little. I hope the attached patch is formatted ok - this is how it came from Mercurial. I applied it using patch -p 1. It worked fine, thanks. I do tend to find -c format more readable than -u, but in a case like this where it's all additions that doesn't make much difference. 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] pg_typeof() patch review
On Nov 3, 2008, at 10:52 AM, Alvaro Herrera wrote: Give this a read http://www.postgresql.org/docs/8.3/static/datatype-oid.html Yeah. Maybe we should link to this page in the pg_typeof() description. Also, perhaps this page needs more examples. Yes, both of those would help a lot, I think. David -- 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] pg_typeof() patch review
David E. Wheeler [EMAIL PROTECTED] writes: On Nov 3, 2008, at 10:52 AM, Alvaro Herrera wrote: Maybe we should link to this page in the pg_typeof() description. Also, perhaps this page needs more examples. Yes, both of those would help a lot, I think. Feel free to send in a docs patch ... 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
