Re: [BUGS] BUG #1588: pg_autovacuum sleep parameter overflow

The following bug has been logged online:

Bug reference: 1588
Logged by: Lionel Bouton
Email address: lionel-subscription(at)bouton(dot)name
PostgreSQL version: 8.0.1
Operating system: Linux 2.6, glibc 2.3.4
Description: pg_autovacuum sleep parameter overflow
Details:

pg_autovacuum doesn't sleep when the -s parameter is set above 2147
(PostgreSQL then is under constant stress), seems like a 32-bit signed
integer overflow (the time in s then gets past 2^31-1) somewhere.

This worked with 7.4.6 (at least with -s 3600).

Lionel Bouton wrote:
>
> The following bug has been logged online:
>
> Bug reference: 1588
> Logged by: Lionel Bouton
> Email address: lionel-subscription(at)bouton(dot)name
> PostgreSQL version: 8.0.1
> Operating system: Linux 2.6, glibc 2.3.4
> Description: pg_autovacuum sleep parameter overflow
> Details:
>
> pg_autovacuum doesn't sleep when the -s parameter is set above 2147
> (PostgreSQL then is under constant stress), seems like a 32-bit signed
> integer overflow (the time in s then gets past 2^31-1) somewhere.
>
> This worked with 7.4.6 (at least with -s 3600).

Yes, this is a bug. Right now, pg_autovacuum uses pg_usleep(), which
accepts a 'long' value representing the number of microseconds to sleep.
Because long is 32-bits on many platforms, it overflows around 2 billion
microseconds, which is 2147 seconds. The comment in pg_usleep()
verifies it:

* On machines where "long" is 32 bits, the maximum delay is ~2000 seconds.

Here is a proposed patch that will fix it. There is no reason to use
pg_usleep in pg_autovacuum in this area, so we just use sleep(). We
don't need micro-second accuracy, and we don't need Win32 to handle
signals that arrive during the sleep.

However, I am now wondering if we should change pg_usleep() to take a
double rather than long. This would avoid such problems in the future
in other places in our code.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:

> */
>! #ifndef WIN32
>! sleep(sleep_secs); /* Unix sleep is seconds */
>! #else
>! sleep(sleep_secs * 1000); /* Win32 sleep() is milliseconds */
>
>

Shouldn't the be Sleep with a capital S? see
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dllproc/base/sleep.asp

cheers

andrew

Andrew Dunstan wrote:
>
>
> Bruce Momjian wrote:
>
> > */
> >! #ifndef WIN32
> >! sleep(sleep_secs); /* Unix sleep is seconds */
> >! #else
> >! sleep(sleep_secs * 1000); /* Win32 sleep() is milliseconds */
> >
> >
>
> Shouldn't the be Sleep with a capital S? see
> http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dllproc/base/sleep.asp

I was wondering about that, so I compiled it with MinGW and sleep()
worked fine, so I stuck with that. In fact, I just tried Sleep() and
that didn't work. It said:

C:/DOCUME~1/BRUCEM~1/LOCALS~1/Temp/ccMLbaaa.o(.text+0x27):x.c: undefined
reference to `Sleep'

Maybe if I added some includes it would work, but sleep() seemed safest.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:

>Andrew Dunstan wrote:
>
>
>>Bruce Momjian wrote:
>>
>>
>>
>>> */
>>>! #ifndef WIN32
>>>! sleep(sleep_secs); /* Unix sleep is seconds */
>>>! #else
>>>! sleep(sleep_secs * 1000); /* Win32 sleep() is milliseconds */
>>>
>>>
>>>
>>>
>>Shouldn't the be Sleep with a capital S? see
>>http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dllproc/base/sleep.asp
>>
>>
>
>I was wondering about that, so I compiled it with MinGW and sleep()
>worked fine, so I stuck with that. In fact, I just tried Sleep() and
>that didn't work. It said:
>
> C:/DOCUME~1/BRUCEM~1/LOCALS~1/Temp/ccMLbaaa.o(.text+0x27):x.c: undefined
> reference to `Sleep'
>
>Maybe if I added some includes it would work, but sleep() seemed safest.
>
>
>

Strange ... as long as you #include <windows.h> it should be fine, and
pg_autovacuum.c already does include it.

Anyway, whatever works, I guess.

cheers

andrew

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> However, I am now wondering if we should change pg_usleep() to take a
> double rather than long. This would avoid such problems in the future
> in other places in our code.

I'd leave it alone; there aren't any other places that need long sleeps,
and I don't really expect them. When and if we have a real need for it,
we can change it.

regards, tom lane

Andrew Dunstan wrote:
> Strange ... as long as you #include <windows.h> it should be fine, and
> pg_autovacuum.c already does include it.
>
> Anyway, whatever works, I guess.
>

Ah, my test program did not include "windows.h". I found it understood
sleep() with just the standard Unix includes. I did not test compiling
pg_autovacuum itself.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Patch applied and backpatched to 8.0.X.

---------------------------------------------------------------------------

Bruce Momjian wrote:

> Index: contrib/pg_autovacuum/pg_autovacuum.c
> ===================================================================
> RCS file: /cvsroot/pgsql/contrib/pg_autovacuum/pg_autovacuum.c,v
> retrieving revision 1.31
> diff -c -c -r1.31 pg_autovacuum.c
> *** contrib/pg_autovacuum/pg_autovacuum.c 19 Apr 2005 03:35:15 -0000 1.31
> --- contrib/pg_autovacuum/pg_autovacuum.c 11 May 2005 14:43:34 -0000
> ***************
> *** 1749,1755 ****
> fflush(LOGOUTPUT);
> }
>
> ! pg_usleep(sleep_secs * 1000000); /* Larger Pause between outer loops */
>
> gettimeofday(&then, 0); /* Reset time counter */
>
> --- 1749,1764 ----
> fflush(LOGOUTPUT);
> }
>
> ! /* Larger Pause between outer loops */
> ! /*
> ! * pg_usleep() is wrong here because its maximum is ~2000 seconds,
> ! * and we don't need signal interruptability on Win32 here.
> ! */
> ! #ifndef WIN32
> ! sleep(sleep_secs); /* Unix sleep is seconds */
> ! #else
> ! sleep(sleep_secs * 1000); /* Win32 sleep() is milliseconds */
> ! #endif
>
> gettimeofday(&then, 0); /* Reset time counter */
>

>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/docs/faq

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073