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
Attachments:
pg_typeof_081103.difftext/plain; name=pg_typeof_081103.diffDownload
diff -r 4b92d79506ba doc/src/sgml/func.sgml
--- a/doc/src/sgml/func.sgml Mon Nov 03 01:17:08 2008 +0000
+++ b/doc/src/sgml/func.sgml Mon Nov 03 00:13:50 2008 -0800
@@ -11592,6 +11592,10 @@
</indexterm>
<indexterm>
+ <primary>pg_typeof</primary>
+ </indexterm>
+
+ <indexterm>
<primary>pg_get_keywords</primary>
</indexterm>
@@ -11660,6 +11664,11 @@
<entry><literal><function>format_type</function>(<parameter>type_oid</parameter>, <parameter>typemod</>)</literal></entry>
<entry><type>text</type></entry>
<entry>get SQL name of a data type</entry>
+ </row>
+ <row>
+ <entry><literal><function>pg_typeof</function>(<parameter>any</parameter>)</literal></entry>
+ <entry><type>regtype</type></entry>
+ <entry>get the data type of any value</entry>
</row>
<row>
<entry><literal><function>pg_get_keywords</function>()</literal></entry>
@@ -11774,6 +11783,12 @@
<function>format_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>
+ <function>pg_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 +0000
+++ 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 +0000
+++ b/src/include/catalog/catversion.h Mon Nov 03 00:13:51 2008 -0800
@@ -53,6 +53,6 @@
*/
/* yyyymmddN */
-#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 +0000
+++ 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_flt4 PGNSP 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 +0000
+++ 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.out Mon Nov 03 01:17:08 2008 +0000
+++ b/src/test/regress/expected/polymorphism.out Mon 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), -- numeric
+ pg_typeof(1+1 = 2), -- boolean
+ pg_typeof('x'), -- unknown
+ pg_typeof('' || ''), -- text
+ pg_typeof(pg_typeof(0)); -- regtype
+ pg_typeof | pg_typeof | pg_typeof | pg_typeof | pg_typeof | pg_typeof | pg_typeof
+-----------+-----------+-----------+-----------+-----------+-----------+-----------
+ unknown | integer | numeric | boolean | unknown | text | regtype
+(1 row)
+
+select pg_typeof(f1), pg_typeof(sql_if(f1 > 0, bleat(f1), bleat(f1 + 1))) from int4_tbl limit 1;
+NOTICE: bleat 1
+ pg_typeof | pg_typeof
+-----------+-----------
+ integer | integer
+(1 row)
+
+select pg_typeof(q2), pg_typeof(sql_if(q2 > 0, q2, q2 + 1)) from int8_tbl limit 1;
+ pg_typeof | pg_typeof
+-----------+-----------
+ bigint | bigint
+(1 row)
+
+select pg_typeof(myleast(10, 1, 20, 33)),
+ pg_typeof(myleast(variadic array[1.1, -5.5])),
+ pg_typeof(formarray(1,2,3,4,5)),
+ pg_typeof(formarray(1.1, variadic array[1.2,55.5])),
+ pg_typeof(concat('%', 1, 2, 3, 4, 5));
+ pg_typeof | pg_typeof | pg_typeof | pg_typeof | pg_typeof
+-----------+-----------+-----------+-----------+-----------
+ integer | numeric | integer[] | numeric[] | text
+(1 row)
+
+select pg_typeof(array[1.2,55.5]),
+ pg_typeof(variadic array[1,2,33]),
+ pg_typeof(variadic array[]::int[]),
+ pg_typeof(row(1,1.1)),
+ pg_typeof(array[ row(7,7.7), row(1,1.0), row(0,0.0) ]);
+ pg_typeof | pg_typeof | pg_typeof | pg_typeof | pg_typeof
+-----------+-----------+-----------+-----------+-----------
+ numeric[] | integer[] | integer[] | record | record[]
+(1 row)
+
+-- cleanup
+drop function concat(text, anyarray);
drop function formarray(anyelement, variadic anyarray);
diff -r 4b92d79506ba src/test/regress/sql/polymorphism.sql
--- a/src/test/regress/sql/polymorphism.sql Mon Nov 03 01:17:08 2008 +0000
+++ b/src/test/regress/sql/polymorphism.sql Mon Nov 03 00:13:51 2008 -0800
@@ -455,8 +455,6 @@
select concat('|', variadic array[1,2,33]);
select concat('|', variadic array[]::int[]);
-drop function concat(text, anyarray);
-
-- mix variadic with anyelement
create function formarray(anyelement, variadic anyarray) returns anyarray as $$
select array_prepend($1, $2);
@@ -468,4 +466,27 @@
select formarray(1, 'x'::text); -- fail, type mismatch
select formarray(1, variadic array['x'::text]); -- fail, type mismatch
+-- test pg_typeof() function
+select pg_typeof(null), -- unknown
+ pg_typeof(0), -- integer
+ pg_typeof(0.0), -- numeric
+ pg_typeof(1+1 = 2), -- boolean
+ pg_typeof('x'), -- unknown
+ pg_typeof('' || ''), -- text
+ pg_typeof(pg_typeof(0)); -- regtype
+select pg_typeof(f1), pg_typeof(sql_if(f1 > 0, bleat(f1), bleat(f1 + 1))) from int4_tbl limit 1;
+select pg_typeof(q2), pg_typeof(sql_if(q2 > 0, q2, q2 + 1)) from int8_tbl limit 1;
+select pg_typeof(myleast(10, 1, 20, 33)),
+ pg_typeof(myleast(variadic array[1.1, -5.5])),
+ pg_typeof(formarray(1,2,3,4,5)),
+ pg_typeof(formarray(1.1, variadic array[1.2,55.5])),
+ pg_typeof(concat('%', 1, 2, 3, 4, 5));
+select pg_typeof(array[1.2,55.5]),
+ pg_typeof(variadic array[1,2,33]),
+ pg_typeof(variadic array[]::int[]),
+ pg_typeof(row(1,1.1)),
+ pg_typeof(array[ row(7,7.7), row(1,1.0), row(0,0.0) ]);
+
+-- cleanup
+drop function concat(text, anyarray);
drop function formarray(anyelement, variadic anyarray);
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
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 stringActually, it returns a regtype, no?
Yes -- regtype, which is an "OID alias type".
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Kurt Harriman <harriman@acm.org> 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
"David E. Wheeler" <david@kineticode.com> 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
On Nov 3, 2008, at 10:02 AM, Tom Lane wrote:
"David E. Wheeler" <david@kineticode.com> 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 stringActually, 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
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 Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
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
"David E. Wheeler" <david@kineticode.com> 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
On Nov 3, 2008, at 11:15 AM, Tom Lane wrote:
"David E. Wheeler" <david@kineticode.com> 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
Attachments:
pg_typeof_doc.patchapplication/octet-stream; name=pg_typeof_doc.patch; x-unix-mode=0644Download
Index: doc/src/sgml/func.sgml
===================================================================
RCS file: /projects/cvsroot/pgsql/doc/src/sgml/func.sgml,v
retrieving revision 1.455
diff -c -w -r1.455 func.sgml
*** doc/src/sgml/func.sgml 4 Nov 2008 14:49:11 -0000 1.455
--- doc/src/sgml/func.sgml 7 Nov 2008 18:31:11 -0000
***************
*** 1,4 ****
! <!-- $PostgreSQL: pgsql/doc/src/sgml/func.sgml,v 1.455 2008/11/04 14:49:11 petere Exp $ -->
<chapter id="functions">
<title>Functions and Operators</title>
--- 1,4 ----
! !-- $PostgreSQL: pgsql/doc/src/sgml/func.sgml,v 1.455 2008/11/04 14:49:11 petere Exp $ -->
<chapter id="functions">
<title>Functions and Operators</title>
***************
*** 11869,11877 ****
</para>
<para>
! <function>pg_typeof</function> returns the OID of the data type of the
value that is passed to it. This can be helpful for troubleshooting or
! dynamically constructing SQL queries.
</para>
<indexterm>
--- 11869,11886 ----
</para>
<para>
! <function>pg_typeof</function> returns the <type>regtype</> (also usable as
! an OID; see <xref linkend="datatype-oid">) of the data type of the
value that is passed to it. This can be helpful for troubleshooting or
! dynamically constructing SQL queries. An example:
! <programlisting>
! SELECT select pg_typeof(33);
!
! pg_typeof
! -----------
! integer
! (1 row)
! </programlisting>
</para>
<indexterm>
"David E. Wheeler" <david@kineticode.com> 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
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