Re: A couple of cosmetic changes around shared memory code

On Wed, Jun 29, 2016 at 4:35 PM, Piotr Stefaniak
<postgres(at)piotr-stefaniak(dot)me> wrote:
> On 2016-06-29 18:58, Robert Haas wrote:
>> This code predates be7558162acc5578d0b2cf0c8d4c76b6076ce352, prior to
>> which proc_exit(0) forced an immediate, unconditional restart. It's
>> true that, given that commit, changing this code to do proc_exit(0)
>> instead of proc_exit(1) would be harmless. However, people writing
>> background workers that might need to work with 9.3 would be best
>> advised to stick with proc_exit(1). Therefore, I maintain that this
>> is not broken and doesn't need to be fixed.
>
> Then I'd argue that it ought to be documented in form of a C comment for
> people writing background workers and for those who might want to "fix" this
> in the future.

Well, I suppose we could do that. Would we then add the same comment
to worker_spi, which does the same thing for the same reason, and
every future contrib module that does stuff with background workers
which we might accept?

It might be better to document this in bgworker.sgml instead. That
already documents some facts about exiting:

<para>
If <structfield>bgw_restart_time</structfield> for a background worker is
configured as <literal>BGW_NEVER_RESTART</>, or if it exits with an exit
code of 0 or is terminated by <function>TerminateBackgroundWorker</>,
it will be automatically unregistered by the postmaster on exit.
Otherwise, it will be restarted after the time period configured via
<structfield>bgw_restart_time</>, or immediately if the postmaster
reinitializes the cluster due to a backend failure. Backends which need
to suspend execution only temporarily should use an interruptible sleep
rather than exiting; 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.
</para>

That paragraph leaves out a number of important details, like:

1. A background worker that exits with any exit code other than 0 or 1
will cause a postmaster crash-and-restart cycle.

2. Therefore, an exit of code 1 should be used whenever the process
wants to be restarted in accordance with bgw_restart_time, and is
therefore in some sense the "normal" way for a background worker to
exit.

3. The aforementioned details about how 9.3 behavior was different
from current behavior.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company