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