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

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 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 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

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

"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