On Thu, Oct 27, 2016 at 7:29 AM, Tsunakawa, Takayuki
<[email protected]> wrote:
> From: [email protected]
>> [mailto:[email protected]] 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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
