8000 Fix "pg_ctl start -w" to test child process status directly. · SudhirLonkar/postgres@c869a7d · GitHub
[go: up one dir, main page]
Skip to content

Commit c869a7d

Browse files
tglsfdctglsfdc
committed
Fix "pg_ctl start -w" to test child process status directly.
pg_ctl start with -w previously relied on a heuristic that the postmaster would surely always manage to create postmaster.pid within five seconds. Unfortunately, that fails much more often than we would like on some of the slower, more heavily loaded buildfarm members. We have known for quite some time that we could remove the need for that heuristic on Unix by using fork/exec instead of system() to launch the postmaster. This allows us to know the exact PID of the postmaster, which allows near-certain verification that the postmaster.pid file is the one we want and not a leftover, and it also lets us use waitpid() to detect reliably whether the child postmaster has exited or not. What was blocking this change was not wanting to rewrite the Windows version of start_postmaster() to avoid use of CMD.EXE. That's doable in theory but would require fooling about with stdout/stderr redirection, and getting the handling of quote-containing postmaster switches to stay the same might be rather ticklish. However, we realized that we don't have to do that to fix the problem, because we can test whether the shell process has exited as a proxy for whether the postmaster is still alive. That doesn't allow an exact check of the PID in postmaster.pid, but we're no worse off than before in that respect; and we do get to get rid of the heuristic about how long the postmaster might take to create postmaster.pid. On Unix, this change means that a second "pg_ctl start -w" immediately after another such command will now reliably fail, whereas previously it would succeed if done within two seconds of the earlier command. Since that's a saner behavior anyway, it's fine. On Windows, the case can still succeed within the same time window, since pg_ctl can't tell that the earlier postmaster's postmaster.pid isn't the pidfile it is looking for. To ensure stable test results on Windows, we can insert a short sleep into the test script for pg_ctl, ensuring that the existing pidfile looks stale. This hack can be removed if we ever do rewrite start_postmaster(), but that no longer seems like a high-priority thing to do. Back-patch to all supported versions, both because the current behavior is buggy and because we must do that if we want the buildfarm failures to go away. Tom Lane and Michael Paquier
1 parent ef5f811 commit c869a7d

File tree

1 file changed

+107
-78
lines changed

1 file changed

+107
-78
lines changed

‎src/bin/pg_ctl/pg_ctl.c

Lines changed: 107 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
#include <time.h>
2929
#include <sys/types.h>
3030
#include <sys/stat.h>
31+
#include <sys/wait.h>
3132
#include <unistd.h>
3233

3334
#ifdef HAVE_SYS_RESOURCE_H
@@ -149,10 +150,10 @@ static int CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo,
149150

150151
static pgpid_t get_pgpid(void);
151152
static char **readfile(const char *path);
152-
static int start_postmaster(void);
153+
static pgpid_t start_postmaster(void);
153154
static void read_post_opts(void);
154155

155-
static PGPing test_postmaster_connection(bool);
156+
static PGPing test_postmaster_connection(pgpid_t pm_pid, bool do_checkpoint);
156157
static bool postmaster_is_alive(pid_t pid);
157158

158159
#if defined(HAVE_GETRLIMIT) && defined(RLIMIT_CORE)
@@ -396,36 +397,73 @@ readfile(const char *path)
396397
* start/test/stop routines
397398
*/
398399

399-
static int
400+
/*
401+
* Start the postmaster and return its PID.
402+
*
403+
* Currently, on Windows what we return is the PID of the shell process
404+
* that launched the postmaster (and, we trust, is waiting for it to exit).
405+
* So the PID is usable for "is the postmaster still running" checks,
406+
* but cannot be compared directly to postmaster.pid.
407+
*
408+
* On Windows, we also save aside a handle to the shell process in
409+
* "postmasterProcess", which the caller should close when done with it.
410+
*/
411+
static pgpid_t
400412
start_postmaster(void)
401413
{
402414
char cmd[MAXPGPATH];
403415

404416
#ifndef WIN32
417+
pgpid_t pm_pid;
418+
419+
/* Flush stdio channels just before fork, to avoid double-output problems */
420+
fflush(stdout);
421+
fflush(stderr);
422+
423+
pm_pid = fork();
424+
if (pm_pid < 0)
425+
{
426+
/* fork failed */
427+
write_stderr(_("%s: could not start server: %s\n"),
428+
progname, strerror(errno));
429+
exit(1);
430+
}
431+
if (pm_pid > 0)
432+
{
433+
/* fork succeeded, in parent */
434+
return pm_pid;
435+
}
436+
437+
/* fork succeeded, in child */
405438

406439
/*
407440
* Since there might be quotes to handle here, it is easier simply to pass
408-
* everything to a shell to process them.
409-
*
410-
* XXX it would be better to fork and exec so that we would know the child
411-
* postmaster's PID directly; then test_postmaster_connection could use
412-
* the PID without having to rel 629A y on reading it back from the pidfile.
441+
* everything to a shell to process them. Use exec so that the postmaster
442+
* has the same PID as the current child process.
413443
*/
414444
if (log_file != NULL)
415-
snprintf(cmd, MAXPGPATH, SYSTEMQUOTE "\"%s\" %s%s < \"%s\" >> \"%s\" 2>&1 &" SYSTEMQUOTE,
445+
snprintf(cmd, MAXPGPATH, "exec \"%s\" %s%s < \"%s\" >> \"%s\" 2>&1",
416446
exec_path, pgdata_opt, post_opts,
417447
DEVNULL, log_file);
418448
else
419-
snprintf(cmd, MAXPGPATH, SYSTEMQUOTE "\"%s\" %s%s < \"%s\" 2>&1 &" SYSTEMQUOTE,
449+
snprintf(cmd, MAXPGPATH, "exec \"%s\" %s%s < \"%s\" 2>&1",
420450
exec_path, pgdata_opt, post_opts, DEVNULL);
421451

422-
return system(cmd);
452+
(void) execl("/bin/sh", "/bin/sh", "-c", cmd, (char *) NULL);
453+
454+
/* exec failed */
455+
write_stderr(_("%s: could not start server: %s\n"),
456+
progname, strerror(errno));
457+
exit(1);
458+
459+
return 0; /* keep dumb compilers quiet */
460+
423461
#else /* WIN32 */
424462

425463
/*
426-
* On win32 we don't use system(). So we don't need to use & (which would
427-
* be START /B on win32). However, we still call the shell (CMD.EXE) with
428-
* it to handle redirection etc.
464+
* As with the Unix case, it's easiest to use the shell (CMD.EXE) to
465+
* handle redirection etc. Unfortunately CMD.EXE lacks any equivalent of
466+
* "exec", so we don't get to find out the postmaster's PID immediately.
429467
*/
430468
PROCESS_INFORMATION pi;
431469

@@ -437,10 +475,15 @@ start_postmaster(void)
437475
exec_path, pgdata_opt, post_opts, DEVNULL);
438476

439477
if (!CreateRestrictedProcess(cmd, &pi, false))
440-
return GetLastError();
441-
CloseHandle(pi.hProcess);
478+
{
479+
write_stderr(_("%s: could not start server: error code %lu\n"),
480+
progname, (unsigned long) GetLastError());
481+
exit(1);
482+
}
483+
/* Don't close command process handle here; caller must do so */
484+
postmasterProcess = pi.hProcess;
442485
CloseHandle(pi.hThread);
443-
return 0;
486+
return pi.dwProcessId; /* Shell's PID, not postmaster's! */
444487
#endif /* WIN32 */
445488
}
446489

@@ -449,15 +492,21 @@ start_postmaster(void)
449492
/*
450493
* Find the pgport and try a connection
451494
*
495+
* On Unix, pm_pid is the PID of the just-launched postmaster. On Windows,
496+
* it may be the PID of an ancestor shell process, so we can't check the
497+
* contents of postmaster.pid quite as carefully.
498+
*
499+
* On Windows, the static variable postmasterProcess is an implicit argument
500+
* to this routine; it contains a handle to the postmaster process or an
501+
* ancestor shell process thereof.
502+
*
452503
* Note that the checkpoint parameter enables a Windows service control
453504
* manager checkpoint, it's got nothing to do with database checkpoints!!
454505
*/
455506
static PGPing
456-
test_postmaster_connection(bool do_checkpoint)
507+
test_postmaster_connection(pgpid_t pm_pid, bool do_checkpoint)
457508
{
458509
PGPing ret = PQPING_NO_RESPONSE;
459-
bool found_stale_pidfile = false;
460-
pgpid_t pm_pid = 0;
461510
char connstr[MAXPGPATH * 2 + 256];
462511
int i;
463512

@@ -512,29 +561,27 @@ test_postmaster_connection(bool do_checkpoint)
512561
optlines[5] != NULL)
513562
{
514563
/* File is complete enough for us, parse it */
515-
long pmpid;
564+
pgpid_t pmpid;
516565
time_t pmstart;
517566

518567
/*
519-
* Make sanity checks. If it's for a standalone backend
520-
* (negative PID), or the recorded start time is before
521-
* pg_ctl started, then either 10000 we are looking at the wrong
522-
* data directory, or this is a pre-existing pidfile that
523-
* hasn't (yet?) been overwritten by our child postmaster.
524-
* Allow 2 seconds slop for possible cross-process clock
525-
* skew.
568+
* Make sanity checks. If it's for the wrong PID, or the
569+
* recorded start time is before pg_ctl started, then
570+
* either we are looking at the wrong data directory, or
571+
* this is a pre-existing pidfile that hasn't (yet?) been
572+
* overwritten by our child postmaster. Allow 2 seconds
573+
* slop for possible cross-process clock skew.
526574
*/
527575
pmpid = atol(optlines[LOCK_FILE_LINE_PID - 1]);
528576
pmstart = atol(optlines[LOCK_FILE_LINE_START_TIME - 1]);
529-
if (pmpid <= 0 || pmstart < start_time - 2)
530-
{
531-
/*
532-
* Set flag to report stale pidfile if it doesn't get
533-
* overwritten before we give up waiting.
534-
*/
535-
found_stale_pidfile = true;
536-
}
537-
else
577+
if (pmstart >= start_time - 2 &&
578+
#ifndef WIN32
579+
pmpid == pm_pid
580+
#else
581+
/* Windows can only reject standalone-backend PIDs */
582+
pmpid > 0
583+
#endif
584+
)
538585
{
539586
/*
540587
* OK, seems to be a valid pidfile from our child.
@@ -544,9 +591,6 @@ test_postmaster_connection(bool do_checkpoint)
544591
char *hostaddr;
545592
char host_str[MAXPGPATH];
546593

547-
found_stale_pidfile = false;
548-
pm_pid = (pgpid_t) pmpid;
549-
550594
/*
551595
* Extract port number and host string to use. Prefer
552596
* using Unix socket if available.
@@ -605,37 +649,23 @@ test_postmaster_connection(bool do_checkpoint)
605649
}
606650

607651
/*
608-
* The postmaster should create postmaster.pid very soon after being
609-
* started. If it's not there after we've waited 5 or more seconds,
610-
* assume startup failed and give up waiting. (Note this covers both
611-
* cases where the pidfile was never created, and where it was created
612-
* and then removed during postmaster exit.) Also, if there *is* a
613-
* file there but it appears stale, issue a suitable warning and give
614-
* up waiting.
652+
* Check whether the child postmaster process is still alive. This
653+
* lets us exit early if the postmaster fails during startup.
654+
*
655+
* On Windows, we may be checking the postmaster's parent shell, but
656+
* that's fine for this purpose.
615657
*/
616-
if (i >= 5)
658+
#ifndef WIN32
617659
{
618-
struct stat statbuf;
619-
620-
if (stat(pid_file, &statbuf) != 0)
621-
return PQPING_NO_RESPONSE;
660+
int exitstatus;
622661

623-
if (found_stale_pidfile)
624-
{
625-
write_stderr(_("\n%s: this data directory appears to be running a pre-existing postmaster\n"),
626-
progname);
662+
if (waitpid((pid_t) pm_pid, &exitstatus, WNOHANG) == (pid_t) pm_pid)
627663
return PQPING_NO_RESPONSE;
628-
}
629664
}
630-
631-
/*
632-
* If we've been able to identify the child postmaster's PID, check
633-
* the process is still alive. This covers cases where the postmaster
634-
* successfully created the pidfile but then crashed without removing
635-
* it.
636-
*/
637-
if (pm_pid > 0 && !postmaster_is_alive((pid_t) pm_pid))
665+
#else
666+
if (WaitForSingleObject(postmasterProcess, 0) == WAIT_OBJECT_0)
638667
return PQPING_NO_RESPONSE;
668+
#endif
639669

640670
/* No response, or startup still in process; wait */
641671
#if defined(WIN32)
@@ -798,7 +828,7 @@ static void
798828
do_start(void)
799829
{
800830
pgpid_t old_pid = 0;
801-
int exitcode;
831+
pgpid_t pm_pid;
802832

803833
if (ctl_command != RESTART_COMMAND)
804834
{
@@ -838,19 +868,13 @@ do_start(void)
838868
}
839869
#endif
840870

841-
exitcode = start_postmaster();
842-
if (exitcode != 0)
843-
{
844-
write_stderr(_("%s: could not start server: exit code was %d\n"),
845-
progname, exitcode);
846-
exit(1);
847-
}
871+
pm_pid = start_postmaster();
848872

849873
if (do_wait)
850874
{
851875
print_msg(_("waiting for server to start..."));
852876

853-
switch (test_postmaster_connection(false))
877+
switch (test_postmaster_connection(pm_pid, false))
854878
{
855879
case PQPING_OK:
856880
print_msg(_(" done\n"));
@@ -876,6 +900,12 @@ do_start(void)
876900
}
877901
else
878902
print_msg(_("server starting\n"));
903+
904+
#ifdef WIN32
905+
/* Now we don't need the handle to the shell process anymore */
906+
CloseHandle(postmasterProcess);
907+
postmasterProcess = INVALID_HANDLE_VALUE;
908+
#endif
879909
}
880910

881911

@@ -1479,7 +1509,7 @@ pgwin32_ServiceMain(DWORD argc, LPTSTR *argv)
14791509
if (do_wait)
14801510
{
14811511
write_eventlog(EVENTLOG_INFORMATION_TYPE, _("Waiting for server startup...\n"));
1482-
if (test_postmaster_connection(true) != PQPING_OK)
1512+
if (test_postmaster_connection(postmasterPID, true) != PQPING_OK)
14831513
{
14841514
write_eventlog(EVENTLOG_ERROR_TYPE, _("Timed out waiting for server startup\n"));
14851515
pgwin32_SetServiceStatus(SERVICE_STOPPED);
@@ -1506,10 +1536,9 @@ pgwin32_ServiceMain(DWORD argc, LPTSTR *argv)
15061536
{
15071537
/*
15081538
* status.dwCheckPoint can be incremented by
1509-
* test_postmaster_connection(true), so it might not
1510-
* start from 0.
1539+
* test_postmaster_connection(), so it might not start from 0.
15111540
*/
1512-
int maxShutdownCheckPoint = status.dwCheckPoint + 12;;
1541+
int maxShutdownCheckPoint = status.dwCheckPoint + 12;
15131542

15141543
kill(postmasterPID, SIGINT);
15151544

0 commit comments

Comments
 (0)
0