From: 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 22:55:42 +0200
Message-ID: <536A9DCE.7060304@2ndquadrant.com> (raw)
In-Reply-To: <CA+TgmoZifBwcv22Gu+Zj-pmZmZG6e-orWuL5SP0_SgHgdHPJ6w@mail.gmail.com>
References: <20140416131052.GG17874@awork2.anarazel.de>
<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>
<CA+TgmoZifBwcv22Gu+Zj-pmZmZG6e-orWuL5SP0_SgHgdHPJ6w@mail.gmail.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
On 07/05/14 22:32, Robert Haas wrote:
> On Tue, May 6, 2014 at 8:25 PM, Petr Jelinek <petr@2ndquadrant.com> wrote:
> I've committed the portion of your patch which does this, with pretty
> extensive changes. I moved the function which resets the crash times
> to bgworker.c, renamed it, and made it just reset all of the crash
> times unconditionally; there didn't seem to be any point in skipping
> the irrelevant ones, so it seemed best to keep things simple. I also
> moved the call from reaper() where you had it to
> PostmasterStateMachine(), because the old placement did not work for
> me when I carried out the following steps:
>
> 1. Kill a background worker with a 60-second restart time using
> SIGTERM, so that it does exit(1).
> 2. Before it restarts, kill a regular backend with SIGQUIT.
>
> Let me know if you think I've got it wrong.
>
No I think it's fine, I didn't try that combination and just wanted to
put it as deep in the call as possible.
>>> (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
>
> I did this as a separate commit,
> e2ce9aa27bf20eff2d991d0267a15ea5f7024cd7, just moving the check for
> ReleasePostmasterChildSlot inside the if statement. Then I realized
> that was bogus, so I just pushed
> eee6cf1f337aa488a20e9111df446cdad770e645 to fix that. Hopefully it's
> OK now.
The fixed one looks ok to me.
>
>>> (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
>
> This isn't done yet.
>
Unless I am missing something this change was included in every patch I
sent - setting rw->rw_terminate = true; in CleanupBackgroundWorker for
zero exit code + comment changes. Or do you have objections to this
approach?
Anyway missing parts attached.
--
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-v4.diff (3.7K, ../536A9DCE.7060304@2ndquadrant.com/2-unregister-bgworker-on-exit0-v4.diff)
download | inline diff:diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgmlindex fd32d6c..5f86100 100644--- a/doc/src/sgml/bgworker.sgml+++ b/doc/src/sgml/bgworker.sgml@@ -166,10 +166,12 @@ typedef struct BackgroundWorker
</para>
<para>
- Background workers are expected to be continuously running; if they exit- cleanly, <command>postgres</> will restart them immediately. Consider doing- interruptible sleep when they have nothing to do; this can be achieved by- calling <function>WaitLatch()</function>. Make sure the+ Background workers are normally expected to be running continuously;+ they get restarted automaticaly in case of crash (see + <structfield>bgw_restart_time</structfield> documentation for details),+ but if they exit cleanly, <command>postgres</> will not restart them.+ Consider doing interruptible sleep when they have nothing to do; this can be+ achieved by calling <function>WaitLatch()</function>. Make sure the
<literal>WL_POSTMASTER_DEATH</> flag is set when calling that function, and
verify the return code for a prompt exit in the emergency case that
<command>postgres</> itself has terminated.
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.cindex 64c9722..b642f37 100644--- a/src/backend/postmaster/bgworker.c+++ b/src/backend/postmaster/bgworker.c@@ -884,8 +884,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 79d1c50..a0331e6 100644--- a/src/backend/postmaster/postmaster.c+++ b/src/backend/postmaster/postmaster.c@@ -2849,7 +2849,11 @@ CleanupBackgroundWorker(int pid,
if (!EXIT_STATUS_0(exitstatus))
rw->rw_crashed_at = GetCurrentTimestamp();
else
+ {+ /* Zero exit status means terminate */
rw->rw_crashed_at = 0;
+ rw->rw_terminate = true;+ }
/*
* Additionally, for shared-memory-connected workers, just like a
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: <536A9DCE.7060304@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