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
postmastermemoryis permitted for static bgworkers, but won't work onEXEC_BACKEND platforms. If any nontrivial data must
bepassed to abgworker, a named shared memory segment should be allocated tocontain it. For multiple workers, the
bgw_main_argmay be an indexinto an array of fixed-width structs in that shm segment.
 
Note that shared memory is re-initialized if the postmaster restartsafter a backend crash. Backends should always use a
sharedmemorystartup hook to initialize their shared memory so that it isre-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