On 09/20/2013 01:59 PM, Fabien COELHO wrote:
>
> Here is a review of the pg_sleep(INTERVAL) patch version 1:
Thank you for looking at it.
>
> - the new function is *not* tested anywhere!
>
> I would suggest simply to replace some pg_sleep(int) instances
> by corresponding pg_sleep(interval) instances in the non
> regression tests.
Attached is a rebased patch that adds a test as you suggest.
> - some concerns have been raised that it breaks pg_sleep(TEXT)
> which currently works thanks to the implicit TEXT -> INT cast.
There is no pg_sleep(text) function and the cast is unknown->double
precision.
>
> I would suggest to add pg_sleep(TEXT) explicitely, like:
>
> CREATE FUNCTION pg_sleep(TEXT) RETURNS VOID VOLATILE STRICT AS
> $$ select pg_sleep($1::INTEGER) $$ LANGUAGE SQL;
>
> That would be another one liner, to update the documentation and
> to add some tests as well!
>
> ISTM that providing "pg_sleep(TEXT)" cleanly resolves the
> upward-compatibility issue raised.
>
I don't like this idea at all. If we don't want to break compatibility
for callers that quote the value, I would rather the patch be rejected
outright.
--
Vik