| From: | Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> |
|---|---|
| To: | Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> |
| Cc: | PG Hackers <pgsql-hackers(at)postgresql(dot)org> |
| Subject: | Re: [PATCH] pg_sleep(interval) |
| Date: | 2013-09-20 11:59:09 |
| Message-ID: | alpine.DEB.2.02.1309201351050.15741@sto |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Here is a review of the pg_sleep(INTERVAL) patch version 1:
- patch applies cleanly on current head
- the function documentation is updated and clear
- the function definition relies on a SQL function
to call the underlying pg_sleep(INT) function
I'm fine with this, especially as if someone calls this
function, he/she is not in a hurry:-)
- this one-liner implementation is straightforward
- the provided feature is simple, and seems useful.
- configure/make/make check work ok
However :
- 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.
- some concerns have been raised that it breaks pg_sleep(TEXT)
which currently works thanks to the implicit TEXT -> INT cast.
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.
--
Fabien
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Robert Haas | 2013-09-20 11:59:30 | Re: Assertions in PL/PgSQL |
| Previous Message | Andres Freund | 2013-09-20 11:57:28 | Re: SSI freezing bug |