Thread: Minor race-condition problem during database startup
Buildfarm member pika showed an interesting transient failure yesterday: http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=pika&dt=2008-11-21%2012:13:06 ./pg_regress --inputdir=. --dlpath=. --multibyte=SQL_ASCII --load-language=plpgsql --no-locale --temp-install=./tmp_check--top-builddir=../../.. --temp-port=55678 --schedule=./parallel_schedule --temp-config=/tmp/buildfarm-24h8Wg ============== creating temporary installation ============== ============== initializing database system ============== ============== starting postmaster ============== running on port 55678 with pid 19223 ============== creating database "regression" ============== ERROR: could not request checkpoint because bgwriter not running command failed: "/home/pgbuildfarm/workdir/HEAD/pgsql.3950/src/test/regress/./tmp_check/install//home/pgbuildfarm/workdir/HEAD/inst/bin/psql" -X-c "CREATE DATABASE \"regression\" TEMPLATE=template0 ENCODING='SQL_ASCII'" "postgres" server stopped gmake: *** [check] Error 2 What seems to have happened is that the bgwriter didn't get as far as the first line of BackgroundWriterMain before the client backend tried to issue a checkpoint request. This is obviously a pretty minor issue, but it still seems worth fixing. We could either try to make sure that BgWriterShmem->bgwriter_pid gets set before the postmaster "opens its doors" for clients, or allow RequestCheckpoint() to wait a little bit if needed for the bgwriter to come ready. The latter seems like a more localized change. Thoughts? regards, tom lane
Tom Lane napsal(a):
> What seems to have happened is that the bgwriter didn't get as far as
> the first line of BackgroundWriterMain before the client backend tried
> to issue a checkpoint request.
>
> This is obviously a pretty minor issue, but it still seems worth fixing.
> We could either try to make sure that BgWriterShmem->bgwriter_pid gets
> set before the postmaster "opens its doors" for clients, or allow
> RequestCheckpoint() to wait a little bit if needed for the bgwriter
> to come ready. The latter seems like a more localized change.
I think, postmaster should wait until bgwriter is not up.
Another strange thing in RequestCheckpoint() is following code:
00926 else if (kill(BgWriterShmem->bgwriter_pid, SIGINT) != 0)
00927 {
00928 if (ntries >= 20) /* max wait 2.0 sec */
00929 {
00930 elog((flags & CHECKPOINT_WAIT) ? ERROR : LOG,
00931 "could not signal for checkpoint: %m");
00932 break;
00933 }
00934 }
By my opinion there is not reason to retry kill call, because it fails only in
situation if process does not exist or caller does not have permission to send a
signal. If one of these situation happens it means that bgwriter is dead or
memory is corrupted. Maybe it is time for panic (or fatal)?
Zdenek
Zdenek Kotala napsal(a):
> Tom Lane napsal(a):
>
>> What seems to have happened is that the bgwriter didn't get as far as
>> the first line of BackgroundWriterMain before the client backend tried
>> to issue a checkpoint request.
>>
>> This is obviously a pretty minor issue, but it still seems worth fixing.
>> We could either try to make sure that BgWriterShmem->bgwriter_pid gets
>> set before the postmaster "opens its doors" for clients, or allow
>> RequestCheckpoint() to wait a little bit if needed for the bgwriter
>> to come ready. The latter seems like a more localized change.
>
> I think, postmaster should wait until bgwriter is not up.
>
> Another strange thing in RequestCheckpoint() is following code:
>
> 00926 else if (kill(BgWriterShmem->bgwriter_pid, SIGINT) != 0)
> 00927 {
> 00928 if (ntries >= 20) /* max wait 2.0 sec */
> 00929 {
> 00930 elog((flags & CHECKPOINT_WAIT) ? ERROR : LOG,
> 00931 "could not signal for checkpoint: %m");
> 00932 break;
> 00933 }
> 00934 }
>
> By my opinion there is not reason to retry kill call, because it fails
> only in situation if process does not exist or caller does not have
> permission to send a signal. If one of these situation happens it means
> that bgwriter is dead or memory is corrupted. Maybe it is time for panic
> (or fatal)?
If I think more about it, that code is here because it probably tries to bypass
time when bgwriter is restarted. But It could invoke another race condition in
situation when old bgwriter pid is recycled to another backend (or another
postgresql server) in mean time. Should postmaster/bgwriter clean
BgWriterShmem->bgwriter_pid at the end? I think it should be zeroed in places
where BgWriterPID is set to zero (in postmaster.c).
Zdenek
Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes: > Zdenek Kotala napsal(a): >> By my opinion there is not reason to retry kill call, because it fails >> only in situation if process does not exist or caller does not have >> permission to send a signal. > If I think more about it, that code is here because it probably tries to bypass > time when bgwriter is restarted. Correct. Did you read the commit log message? http://archives.postgresql.org/pgsql-committers/2008-11/msg00269.php The bgwriter-exit case is currently treated as a crash and I'm not immediately proposing to change that, but this code or something close to it will be needed if we ever do want to recover from bgwriter exit nicely. regards, tom lane
Tom Lane napsal(a): > Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes: >> Zdenek Kotala napsal(a): >>> By my opinion there is not reason to retry kill call, because it fails >>> only in situation if process does not exist or caller does not have >>> permission to send a signal. > >> If I think more about it, that code is here because it probably tries to bypass >> time when bgwriter is restarted. > > Correct. Did you read the commit log message? > http://archives.postgresql.org/pgsql-committers/2008-11/msg00269.php No. I thought that I look on old code. :( Zdenek