From: Petr Jelinek <petr@2ndquadrant.com>
To: Petr Jelinek <petr@2ndquadrant.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Craig Ringer <craig@2ndquadrant.com>
Cc: Andres Freund <andres@2ndquadrant.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Antonin Houska <antonin.houska@gmail.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Cc: Abhijit Menon-Sen <ams@2ndquadrant.com>
Subject: Re: bgworker crashed or not?
Date: Wed, 07 May 2014 16:25:46 +0200
Message-ID: <536A426A.9080708@2ndquadrant.com> (raw)
In-Reply-To: <53697D84.1050103@2ndquadrant.com>
References: <20140416131052.GG17874@awork2.anarazel.de>
<CA+TgmoZPrzktkbwYRgv2fbhBi2_ty9BEtV6Gk-SSrERROZUkvw@mail.gmail.com>
<20140416154412.GL17874@awork2.anarazel.de>
<27214.1397663665@sss.pgh.pa.us>
<20140416160047.GM17874@awork2.anarazel.de>
<CA+TgmoZe4zqRAX8rgpy9UWy=PE7=3B_1J34bTehEi25hCgr3kw@mail.gmail.com>
<20140416161143.GN17874@awork2.anarazel.de>
<CA+TgmoaefVEJeO7+D=VE35Aj2fkgZ3YVqWx8Svzd3v493iJSvg@mail.gmail.com>
<20140416162837.GO17874@awork2.anarazel.de>
<CA+TgmoZLwmkZ2O6T29qAb9EhO-9cyz4Rj9mjLQVkFe673CooWA@mail.gmail.com>
<534EEC61.7000006@2ndquadrant.com>
<534F21E6.7080705@2ndquadrant.com>
<5358A38F.50808@2ndquadrant.com>
<CA+TgmoY0FXMayR1HH+uD+hM8+CL5Xh8nf610Eps55oJSmsU5+A@mail.gmail.com>
<535EB7CB.3020807@2ndquadrant.com>
<CA+Tgmob4Ur9b4opDJXVBpDSNSGpp5V-zG2V4Vt7RdTLT-ZAuLQ@mail.gmail.com>
<5366474D.4060408@2ndquadrant.com>
<CA+TgmoZyW_xVDyew0pNVTe43VbNhPMp2kGi+kcw+f7fM-GXy7w@mail.gmail.com>
<53697D84.1050103@2ndquadrant.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
On 07/05/14 02:25, Petr Jelinek wrote:
> On 06/05/14 19:05, Robert Haas wrote:
>> What I'm inclined to do is change the logic so that:
>>
>> (1) After a crash-and-restart sequence, zero rw->rw_crashed_at, so
>> that anything which is still registered gets restarted immediately.
>
> Yes, that's quite obvious change which I missed completely :).
>
>> (2) If a shmem-connected backend fails to release the deadman switch
>> or exits with an exit code other than 0 or 1, we crash-and-restart. A
>> non-shmem-connected backend never causes a crash-and-restart.
>
> +1
>
>> (3) When a background worker exits without triggering a
>> crash-and-restart, an exit code of precisely 0 causes the worker to be
>> unregistered; any other exit code has no special effect, so
>> bgw_restart_time controls.
>
> +1
>
Ok, attached patch is my try at the proposed changes.
I don't like the reset_bgworker_crash_state function name, maybe you'll
come up with something better...
I passes my tests for the desired behavior, hopefully I didn't miss some
scenario.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackersAttachments:
[text/x-patch] unregister-bgworker-on-exit0-v3.diff (6.2K, ../536A426A.9080708@2ndquadrant.com/2-unregister-bgworker-on-exit0-v3.diff)
download | inline diff:diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.cindex a6b25d8..e913395 100644--- a/src/backend/postmaster/bgworker.c+++ b/src/backend/postmaster/bgworker.c@@ -472,13 +472,13 @@ bgworker_quickdie(SIGNAL_ARGS)
on_exit_reset();
/*
- * Note we do exit(0) here, not exit(2) like quickdie. The reason is that- * we don't want to be seen this worker as independently crashed, because- * then postmaster would delay restarting it again afterwards. If some- * idiot DBA manually sends SIGQUIT to a random bgworker, the "dead man- * switch" will ensure that postmaster sees this as a crash.+ * Note we do exit(2) here, not exit(0) for same reasons quickdie does.+ * Another reason is that bgworker is not restarted after exit(0).+ *+ * The bgw_restart_time does not apply here and the bgworker will be+ * restarted immediately.
*/
- exit(0);+ exit(2);
}
/*
@@ -832,8 +832,9 @@ RegisterDynamicBackgroundWorker(BackgroundWorker *worker,
* running but is no longer.
*
* In the latter case, the worker may be stopped temporarily (if it is
- * configured for automatic restart, or if it exited with code 0) or gone- * for good (if it is configured not to restart and exited with code 1).+ * configured for automatic restart and exited with code 1) or gone for+ * good (if it exited with code other than 1 or if it is configured not+ * to restart).
*/
BgwHandleStatus
GetBackgroundWorkerPid(BackgroundWorkerHandle *handle, pid_t *pidp)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.cindex 6d09887..99d5e85 100644--- a/src/backend/postmaster/postmaster.c+++ b/src/backend/postmaster/postmaster.c@@ -396,6 +396,7 @@ static void TerminateChildren(int signal);
static int CountChildren(int target);
static int CountUnconnectedWorkers(void);
+static void reset_bgworker_crash_state(void);
static void maybe_start_bgworker(void);
static bool CreateOptsFile(int argc, char *argv[], char *fullprogname);
static pid_t StartChildProcess(AuxProcType type);
@@ -2616,7 +2617,10 @@ reaper(SIGNAL_ARGS)
if (PgStatPID == 0)
PgStatPID = pgstat_start();
- /* some workers may be scheduled to start now */+ /* reset the state of crashed bgworkers so their restart is not delayed */+ reset_bgworker_crash_state();++ /* workers may be scheduled to start now */
maybe_start_bgworker();
/* at this point we are really open for business */
@@ -2845,14 +2849,8 @@ CleanupBackgroundWorker(int pid,
snprintf(namebuf, MAXPGPATH, "%s: %s", _("worker process"),
rw->rw_worker.bgw_name);
- /* Delay restarting any bgworker that exits with a nonzero status. */- if (!EXIT_STATUS_0(exitstatus))- rw->rw_crashed_at = GetCurrentTimestamp();- else- rw->rw_crashed_at = 0;-
/*
- * Additionally, for shared-memory-connected workers, just like a+ * For shared-memory-connected workers, just like a
* backend, any exit status other than 0 or 1 is considered a crash
* and causes a system-wide restart.
*/
@@ -2860,21 +2858,21 @@ CleanupBackgroundWorker(int pid,
{
if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_1(exitstatus))
{
- rw->rw_crashed_at = GetCurrentTimestamp();+ rw->rw_crashed_at = 0;
HandleChildCrash(pid, exitstatus, namebuf);
return true;
}
}
- if (!ReleasePostmasterChildSlot(rw->rw_child_slot))- {- /*- * Uh-oh, the child failed to clean itself up. Treat as a crash- * after all.- */+ /* Mark any bgworker that exits with a nonzero status or fails to clean+ * up after itself as crashed and ready for restart at bgw_restart_time. */+ if (!EXIT_STATUS_0(exitstatus) || !ReleasePostmasterChildSlot(rw->rw_child_slot))
rw->rw_crashed_at = GetCurrentTimestamp();
- HandleChildCrash(pid, exitstatus, namebuf);- return true;+ else+ {+ /* Exit status zero means terminate */+ rw->rw_crashed_at = 0;+ rw->rw_terminate = true;
}
/* Get it out of the BackendList and clear out remaining data */
@@ -5463,6 +5461,38 @@ assign_backendlist_entry(RegisteredBgWorker *rw)
}
/*
+ * Reset background worker crash state.+ *+ * Used after the crash-and-restart procedure to make sure+ * all the background workers are started asap.+ */+static void+reset_bgworker_crash_state(void)+{+ slist_mutable_iter iter;++ slist_foreach_modify(iter, &BackgroundWorkerList)+ {+ RegisteredBgWorker *rw;++ rw = slist_container(RegisteredBgWorker, rw_lnode, iter.cur);++ /*+ * We only care about background workers that are not running+ * and are not marked for removal+ */+ if (rw->rw_pid != 0 || rw->rw_terminate)+ continue;++ /*+ * Reset the crash time so that subsequent maybe_start_bgworker+ * call will start the bgworker immediately+ */+ rw->rw_crashed_at = 0;+ }+}++/*
* If the time is right, start one background worker.
*
* As a side effect, the bgworker control variables are set or reset whenever
diff --git a/src/include/postmaster/bgworker.h b/src/include/postmaster/bgworker.hindex c9550cc..d3dd1f2 100644--- a/src/include/postmaster/bgworker.h+++ b/src/include/postmaster/bgworker.h@@ -16,10 +16,11 @@
* that the failure can only be transient (fork failure due to high load,
* memory pressure, too many processes, etc); more permanent problems, like
* failure to connect to a database, are detected later in the worker and dealt
- * with just by having the worker exit normally. A worker which exits with a- * return code of 0 will be immediately restarted by the postmaster. A worker- * which exits with a return code of 1 will be restarted after the configured- * restart interval, or never if that interval is set to BGW_NEVER_RESTART.+ * with just by having the worker exit normally. A worker which exits with+ * a return code of 0 will never be restarted and will be removed from worker+ * list. A worker which exits with a return code of 1 will be restarted after+ * the configured restart interval, or never if that interval is set to+ * BGW_NEVER_RESTART.
* The TerminateBackgroundWorker() function can be used to terminate a
* dynamically registered background worker; the worker will be sent a SIGTERM
* and will not be restarted after it exits. Whenever the postmaster knows
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: petr@2ndquadrant.com, robertmhaas@gmail.com, craig@2ndquadrant.com, andres@2ndquadrant.com, tgl@sss.pgh.pa.us, antonin.houska@gmail.com, ams@2ndquadrant.com
Subject: Re: bgworker crashed or not?
In-Reply-To: <536A426A.9080708@2ndquadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox