Re: BGWorkers, shared memory pointers, and postmaster restart

On 04/16/2014 07:21 PM, Andres Freund wrote:
> On 2014-04-16 19:11:37 +0800, Craig Ringer wrote:
>> On 04/16/2014 02:37 PM, Craig Ringer wrote:
>>> Hi all
>>>
>>> I've been using the dynamic BGWorker support for some recent work, and I
>>> think I've found an issue with how postmaster restarts are handled.
>>>
>>> TL;DR: I don't think there's a safe way to use a BGWorker (static or
>>> dynamic) with bgw_restart_time != BGW_NEVER_RESTART and a bgw_main_arg
>>> Datum that points into shared memory, and think we might need a API
>>> change to fix that.
>>
>> Andres sensibly points out that part of this is easily solved by passing
>> the bgworker an index into an array in a named shmem block. A simple
>> pass-by-value Datum that can be turned into a pointer to a shmem struct.
>>
>> This still doesn't solve the other half of the issue, which is how to
>> handle dynamic bgworkers after a postmaster restart. They're effectively
>> lost/leaked: there's no way to retain a bgworker handle across restart,
>> and no way to list bgworkers, nor is there any idempotent way to say
>> "Start a worker to do <x> only if it doesn't already exist" (unique
>> names, magic cookie hashes, whatever).
>>
>> With the current API the only solution to the second half that I see is
>> to have bgworkers run in non-restart mode and manage them yourself. Not
>> ideal.
>>
>> Instead we need one of:
>>
>> - A flag like BGW_UNREGISTER_ON_RESTART;
>>
>> - To always unregister dynamic bgws on postmaster shm clear + restart;
>>
>> - A way to list bgws, inspect their BackgroundWorker structs and obtain
>> their handles; or
>>
>> - A way to idempotently register a bgw only if it doesn't already exist
>
> I think we should go for always unregistering dynamic bgws. There's
> really little justification for keeping them around after a crash cycle.

Seems simplest too, and extensions can reasonably expect to have to
relaunch things after postmaster restart. We just have to document
*where* they should do that, and that only dynamic bgworkers get cleared
on postmaster restart, not static ones.

I'd like to propose the following text as an addition to the comment in
bgworker.h:

The bgw_main_arg passed to a bgworker should be pass-by-value Datum
(and not a pointer). A pointer to TopMemoryContext postmaster
memory is permitted for static bgworkers, but won't work on
EXEC_BACKEND platforms. If any nontrivial data must be passed to a
bgworker, a named shared memory segment should be allocated to
contain it. For multiple workers, the bgw_main_arg may be an index
into an array of fixed-width structs in that shm segment.

Note that shared memory is re-initialized if the postmaster restarts
after a backend crash. Backends should always use a shared memory
startup hook to initialize their shared memory so that it is
re-initialized on postmaster restart.

Don't think there's any good example code on how to do this in-tree, and
it's too long for a sane comment. Wonder if I should add an example to
contrib/worker_spi/ .

There's no race anywhere btw. bgworkers are killed, then shm is cleared,
then shm callbacks are rerun, then bgworkers are launched. So long as
shm is reinited appropriately by an extension then static bgworkers that
refer to that shm should be OK.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services