On Thu, Oct 27, 2016 at 7:29 AM, Tsunakawa, Takayuki <tsunakawa.ta...@jp.fujitsu.com> wrote: > From: pgsql-hackers-ow...@postgresql.org >> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Ashutosh Bapat >> In pgstat_quickdie(), I think a call to sigaddset(&BlockSig, SIGQUIT) is >> missing before PG_SETMASK(). Although there are some SIGQUIT handlers which >> do not have that call. But I guess, it will be safer to have it. > > I didn't add it because pgstat_quickdie() just exits, like some other > postmaster children. I thought those processes which are concerned about > their termination processing call sigaddset(SIGQUIT), so I went after the > processes who aren't. Is this really necessary? >
Ok. In that case, I think we shouldn't even call PG_SETMASK() similar to pgarch_exit(). Attached patch removes PG_SETMASK(). Let me know if it looks good. >> Also, many other SIGQUIT handlers like bgworker_quickdie() call >> on_exit_reset() followed by exit(2) instead of just exit(1) in >> pgstat_quickdie(). Why is this difference? > > As Robert and Tom said, either exit(1) or exit(2) is OK because reaper() > handles non-zero exit code the same. Yes, per reaper(). 2955 /* 2956 * Was it the statistics collector? If so, just try to start a new 2957 * one; no need to force reset of the rest of the system. (If fail, 2958 * we'll try again in future cycles of the main loop.) 2959 */ 2960 if (pid == PgStatPID) 2961 { 2962 PgStatPID = 0; 2963 if (!EXIT_STATUS_0(exitstatus)) 2964 LogChildExit(LOG, _("statistics collector process"), 2965 pid, exitstatus); 2966 if (pmState == PM_RUN) 2967 PgStatPID = pgstat_start(); 2968 continue; 2969 } > Regarding on_proc_reset(), stats collector is not attached to the shared > memory and does not register on_proc_exit() callbacks. These situations are > the same as the archiver process, so I followed it. > Right. I got confused because of on_shmem_exit() call in pgstat_initialize. But that's per backend code and not executed within stats collector. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c index 5112d6d..b160f4c 100644 --- a/src/backend/postmaster/pgstat.c +++ b/src/backend/postmaster/pgstat.c @@ -244,20 +244,21 @@ static instr_time total_func_time; /* ---------- * Local function forward declarations * ---------- */ #ifdef EXEC_BACKEND static pid_t pgstat_forkexec(void); #endif NON_EXEC_STATIC void PgstatCollectorMain(int argc, char *argv[]) pg_attribute_noreturn(); +static void pgstat_quickdie(SIGNAL_ARGS); static void pgstat_exit(SIGNAL_ARGS); static void pgstat_beshutdown_hook(int code, Datum arg); static void pgstat_sighup_handler(SIGNAL_ARGS); static PgStat_StatDBEntry *pgstat_get_db_entry(Oid databaseid, bool create); static PgStat_StatTabEntry *pgstat_get_tab_entry(PgStat_StatDBEntry *dbentry, Oid tableoid, bool create); static void pgstat_write_statsfiles(bool permanent, bool allDbs); static void pgstat_write_db_statsfile(PgStat_StatDBEntry *dbentry, bool permanent); static HTAB *pgstat_read_statsfiles(Oid onlydb, bool permanent, bool deep); @@ -3694,27 +3695,27 @@ pgstat_send_bgwriter(void) */ NON_EXEC_STATIC void PgstatCollectorMain(int argc, char *argv[]) { int len; PgStat_Msg msg; int wr; /* * Ignore all signals usually bound to some action in the postmaster, - * except SIGHUP and SIGQUIT. Note we don't need a SIGUSR1 handler to - * support latch operations, because we only use a local latch. + * except SIGHUP, SIGTERM and SIGQUIT. Note we don't need a SIGUSR1 + * handler to support latch operations, because we only use a local latch. */ pqsignal(SIGHUP, pgstat_sighup_handler); pqsignal(SIGINT, SIG_IGN); - pqsignal(SIGTERM, SIG_IGN); - pqsignal(SIGQUIT, pgstat_exit); + pqsignal(SIGTERM, pgstat_exit); + pqsignal(SIGQUIT, pgstat_quickdie); pqsignal(SIGALRM, SIG_IGN); pqsignal(SIGPIPE, SIG_IGN); pqsignal(SIGUSR1, SIG_IGN); pqsignal(SIGUSR2, SIG_IGN); pqsignal(SIGCHLD, SIG_DFL); pqsignal(SIGTTIN, SIG_DFL); pqsignal(SIGTTOU, SIG_DFL); pqsignal(SIGCONT, SIG_DFL); pqsignal(SIGWINCH, SIG_DFL); PG_SETMASK(&UnBlockSig); @@ -3724,40 +3725,40 @@ PgstatCollectorMain(int argc, char *argv[]) */ init_ps_display("stats collector process", "", "", ""); /* * Read in existing stats files or initialize the stats to zero. */ pgStatRunningInCollector = true; pgStatDBHash = pgstat_read_statsfiles(InvalidOid, true, true); /* - * Loop to process messages until we get SIGQUIT or detect ungraceful + * Loop to process messages until we get SIGTERM or detect ungraceful * death of our parent postmaster. * * For performance reasons, we don't want to do ResetLatch/WaitLatch after * every message; instead, do that only after a recv() fails to obtain a * message. (This effectively means that if backends are sending us stuff * like mad, we won't notice postmaster death until things slack off a * bit; which seems fine.) To do that, we have an inner loop that * iterates as long as recv() succeeds. We do recognize got_SIGHUP inside * the inner loop, which means that such interrupts will get serviced but * the latch won't get cleared until next time there is a break in the * action. */ for (;;) { /* Clear any already-pending wakeups */ ResetLatch(MyLatch); /* - * Quit if we get SIGQUIT from the postmaster. + * Quit if we get SIGTERM from the postmaster. */ if (need_exit) break; /* * Inner loop iterates as long as we keep getting messages, or until * need_exit becomes set. */ while (!need_exit) { @@ -3940,20 +3941,28 @@ PgstatCollectorMain(int argc, char *argv[]) * Save the final stats to reuse at next startup. */ pgstat_write_statsfiles(true, true); exit(0); } /* SIGQUIT signal handler for collector process */ static void +pgstat_quickdie(SIGNAL_ARGS) +{ + /* Nothing to do for SIGQUIT, just die ... */ + exit(1); +} + +/* SIGTERM signal handler for collector process */ +static void pgstat_exit(SIGNAL_ARGS) { int save_errno = errno; need_exit = true; SetLatch(MyLatch); errno = save_errno; } diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 2d43506..b5b82aa 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -2869,21 +2869,21 @@ reaper(SIGNAL_ARGS) */ SignalChildren(SIGUSR2); pmState = PM_SHUTDOWN_2; /* * We can also shut down the stats collector now; there's * nothing left for it to do. */ if (PgStatPID != 0) - signal_child(PgStatPID, SIGQUIT); + signal_child(PgStatPID, SIGTERM); } else { /* * Any unexpected exit of the checkpointer (including FATAL * exit) is treated as a crash. */ HandleChildCrash(pid, exitstatus, _("checkpointer process")); }
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers