Re: pg_typeof() patch review

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