Re: [PATCH] pg_sleep(interval)

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