Review: Patch for phypot - Pygmy Hippotause - Mailing list pgsql-hackers

This is a review of the phypot - Pygmy Hippotause patch:
http://archives.postgresql.org/message-id/4A9897E1.8090703@netspace.net.au
submitted by Paul Matthews.

Contents & Purpose
==================
The purpose of the patch is to compute a hypotenuse with higher
precision than the current implementation (the HYPOT macro in
src/include/utils/geo_decls.h).  The initial impetus for the patch
goes back to this message
[http://archives.postgresql.org/pgsql-hackers/2009-08/msg01579.php].
The new phypot function (in src/backend/utils/adt/geo_ops.c) is well
documented in the function header comments and matches the discussion
on the wikipedia page [http://en.wikipedia.org/wiki/Hypot].  It is
envisioned that the new phypot function will eventually be replaced by
the standard C99 hypot function.  This message
[http://archives.postgresql.org/pgsql-hackers/2009-08/msg01580.php]
discusses why the standard c99 hypot function can't be used
(PostgreSQL targets c89, not c99 -- although other messages in the
thread make it sound like the hypot function is ubiquitous).

Initial Run
===========
The patch is in context diff form and applies cleanly to the current CVS HEAD.
There are no tests.
There is no documentation, outside of the code comments, as this is an
internal function.

A couple of nitpicking items:
(1) the phypot function is declared as static, but it is not defined that way
(2) to better match the style of the rest of the geo_ops.c file:
        (a) put the function return type on its own line
        (b) don't put spaces after a "(" and before a ")" [e.g.,
if-statements, function declaration]
        (c) put a space between the keyword "if" and the opening "("
        (d) put spaces around arithmetic operators

Performance
===========
The two concerns about the patch in the mail archives
[http://archives.postgresql.org/message-id/4B83EE31020000250002F55E@gw.wicourts.gov]
are that
(1) since there is more logic in the new function than in the original
marco, it might be slower; and
(2) despite the fact that it is a better implementation of the
hypotenuse functionality, it might break existing applications which
are depending on the existing computation

For (1), I wrote a small c program that executed the original HYPOT
macro in a loop 100 million times and I did the same with the new
phypot function.  The new phypot function is, on average, about twice
as slow as the original HYPOT macro.  The HYPOT macro executed 100
million times in 11 seconds and the phypot function executed the same
number of times in 22 seconds.  With both -O2 and -O3, the HYPOT macro
executed in 8 seconds and the phypot in 18.

For (2), I wrote a small c program that executed the original HYPOT
macro and the new phypot function in a loop 100 million times on
random numbers and compared at what precision the two calculations
started to differ.  I found that the difference in the two
calculations were always less than 0.000001.  However, about a third
of the calculations differed at one more magnitude of precision (that
is, there were differences in the calculations that were greater than
0.0000001).

Conclusion
==========
I like that the patch provides greater precision.  However, I am
unclear as to how significant the slow down is in the new function
(it's certainly not very significant for small iterations), nor how
significant the difference in the calculations is between the existing
macro and the new function.

Thanks
Andrew