Re: pg_typeof() patch review

Kurt Harriman <harriman(at)acm(dot)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