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

*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***************
*** 7586,7599 **** SELECT TIMESTAMP 'now';  -- incorrect for use with DEFAULT
     </indexterm>
  
     <para>
!     The following function is available to delay execution of the server
      process:
  <synopsis>
  pg_sleep(<replaceable>seconds</replaceable>)
  </synopsis>
  
      <function>pg_sleep</function> makes the current session's process
!     sleep until <replaceable>seconds</replaceable> seconds have
      elapsed.  <replaceable>seconds</replaceable> is a value of type
      <type>double precision</>, so fractional-second delays can be specified.
      For example:
--- 7586,7601 ----
     </indexterm>
  
     <para>
!     The following functions are available to delay execution of the server
      process:
  <synopsis>
  pg_sleep(<replaceable>seconds</replaceable>)
+ pg_sleep(<replaceable>interval</replaceable>)
  </synopsis>
  
      <function>pg_sleep</function> makes the current session's process
!     sleep until <replaceable>seconds</replaceable> seconds (or the specified
!     <replaceable>interval</replaceable>) have
      elapsed.  <replaceable>seconds</replaceable> is a value of type
      <type>double precision</>, so fractional-second delays can be specified.
      For example:
*** a/src/include/catalog/pg_proc.h
--- b/src/include/catalog/pg_proc.h
***************
*** 3017,3022 **** DATA(insert OID = 2625 ( pg_ls_dir			PGNSP PGUID 12 1 1000 0 0 f f f f t t v 1 0
--- 3017,3024 ----
  DESCR("list all files in a directory");
  DATA(insert OID = 2626 ( pg_sleep			PGNSP PGUID 12 1 0 0 0 f f f f t f v 1 0 2278 "701" _null_ _null_ _null_ _null_ pg_sleep _null_ _null_ _null_ ));
  DESCR("sleep for the specified time in seconds");
+ DATA(insert OID = 3179 ( pg_sleep			PGNSP PGUID 14 1 0 0 0 f f f f t f v 1 0 2278 "1186" _null_ _null_ _null_ _null_ "select pg_sleep(extract(epoch from now() + $1) - extract(epoch from now()))" _null_ _null_ _null_ ));
+ DESCR("sleep for the specified interval");
  
  DATA(insert OID = 2971 (  text				PGNSP PGUID 12 1 0 0 0 f f f f t f i 1 0 25 "16" _null_ _null_ _null_ _null_ booltext _null_ _null_ _null_ ));
  DESCR("convert boolean to text");
*** a/src/test/regress/expected/stats.out
--- b/src/test/regress/expected/stats.out
***************
*** 18,24 **** SET enable_indexscan TO on;
  SET enable_indexonlyscan TO off;
  -- wait to let any prior tests finish dumping out stats;
  -- else our messages might get lost due to contention
! SELECT pg_sleep(2.0);
   pg_sleep 
  ----------
   
--- 18,24 ----
  SET enable_indexonlyscan TO off;
  -- wait to let any prior tests finish dumping out stats;
  -- else our messages might get lost due to contention
! SELECT pg_sleep(interval '2 seconds');
   pg_sleep 
  ----------
   
*** a/src/test/regress/sql/stats.sql
--- b/src/test/regress/sql/stats.sql
***************
*** 16,22 **** SET enable_indexonlyscan TO off;
  
  -- wait to let any prior tests finish dumping out stats;
  -- else our messages might get lost due to contention
! SELECT pg_sleep(2.0);
  
  -- save counters
  CREATE TEMP TABLE prevstats AS
--- 16,22 ----
  
  -- wait to let any prior tests finish dumping out stats;
  -- else our messages might get lost due to contention
! SELECT pg_sleep(interval '2 seconds');
  
  -- save counters
  CREATE TEMP TABLE prevstats AS
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to