Re: dynamic background workers, round two - Mailing list pgsql-hackers

Hi Robert,

On 2013-08-17 12:08:17 -0400, Robert Haas wrote:
> On Sun, Aug 11, 2013 at 1:31 AM, Andres Freund <andres@anarazel.de> wrote:
> > So, I'd suggest something like:
> >
> > typedef enum BgwHandleStatus {
> >    BGWH_SUCCESS, /* sucessfully got status */
> >    BGWH_NOT_YET, /* worker hasn't started yet */
> >    BGWH_GONE, /* worker had been started, but shut down already */
> >    BGWH_POSTMASTER_DIED /* well, there you go */
> > } BgwHandleStatus;
> >
> >
> > BgwHandleStatus GetBackgroundWorkerPid(BackgroundWorkerHandle *handle, pid_t *pid);
> > BgwHandleStatus WaitForBackgroundWorkerStartup(BackgroundWorkerHandle *handle, pid_t *pid);
> 
> OK, here's a patch that API.  I renamed the constants a bit, because a
> process that has stopped is not necessarily gone; it could be
> configured for restart.  But we can say that it is stopped, at the
> moment.
> 
> I'm not sure that this API is an improvement.  But I think it's OK, if
> you prefer it.

Thanks, looks more readable to me. I like code that tells me what it
does without having to look in other places ;)

> +  <function>RegisterDynamicBackgroundWorker(<type>BackgroundWorker
> +  *worker, BackgroundWorkerHandle **handle</type>)</function>.  Unlike
> +  <function>RegisterBackgroundWorker</>, which can only be called from within
> +  the postmaster, <function>RegisterDynamicBackgroundWorker</function> must be
> +  called from a regular backend.
>   </para>

s/which can only be called from within the posmaster/during initial
startup, via shared_preload_libraries/?

>    <para>
> +   When a background worker is registered using the
> +   <function>RegisterDynamicBackgroundWorker</function> function, it is
> +   possible for the backend performing the registration to obtain information
> +   the status of the worker.  Backends wishing to do this should pass the
> +   address of a <type>BackgroundWorkerHandle *</type> as the second argument
> +   to <function>RegisterDynamicBackgroundWorker</function>.  If the worker
> +   is successfully registered, this pointer will be initialized with an
> +   opaque handle that can subsequently be passed to
> +   <function>GetBackgroundWorkerPid(<parameter>BackgroundWorkerHandle *</parameter>, <parameter>pid_t
*</parameter>)</function>.
> +   This function can be used to poll the status of the worker: a return
> +   value of <literal>BGWH_NOT_YET_STARTED</> indicates that the worker has not
> +   yet been started by the postmaster; <literal>BGWH_STOPPED</literal>
> +   indicates that it has been started but is no longer running; and
> +   <literal>BGWH_STATED</literal> indicates that it is currently running.
> +   In this last case, the PID will also be returned via the second argument.
> +  </para>

s/STATED/STARTED/

> diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
> index f7ebd1a..6414291 100644
> --- a/src/backend/commands/async.c
> +++ b/src/backend/commands/async.c

> -#define InvalidPid                (-1)
> -

Hrmpf ;)

>  bool
> -RegisterDynamicBackgroundWorker(BackgroundWorker *worker)
> +RegisterDynamicBackgroundWorker(BackgroundWorker *worker,
> +                                BackgroundWorkerHandle **handle)
>  {

Hm. Not this patches fault, but We seem to allow bgw_start_time ==
BgWorkerStart_PostmasterStart here which doesn't make sense...

> +/*
> + * Get the PID of a dynamically-registered background worker.
> + *
> + * If the return value is InvalidPid, it indicates that the worker has not
> + * yet been started by the postmaster.
> + *
> + * If the return value is 0, it indicates that the worker was started by
> + * the postmaster but is no longer running.
> + *
> + * Any other return value is the PID of the running worker.
> + */
> +BgwHandleStatus
> +GetBackgroundWorkerPid(BackgroundWorkerHandle *handle, pid_t *pidp)
> +{
> +    BackgroundWorkerSlot *slot;
> +    pid_t    pid;
> +
> +    slot = &BackgroundWorkerData->slot[handle->slot];

Maybe add something like Assert(hanlde->slot < max_worker_processes);?

> +/*
> + * Wait for a background worker to start up.
> + *
> + * The return value is the same as for GetBackgroundWorkerPID, except that
> + * we only return InvalidPid if the postmaster has died (and therefore we
> + * know that the worker will never be started).
> + */
> +BgwHandleStatus
> +WaitForBackgroundWorkerStartup(BackgroundWorkerHandle *handle, pid_t *pidp)
> +{
> +    BgwHandleStatus    status;
> +    pid_t    pid;
> +    int        rc;
> +
> +    set_latch_on_sigusr1 = true;
> +
> +    PG_TRY();
> +    {
> +        for (;;)
> +        {
> +            status = GetBackgroundWorkerPid(handle, &pid);
> +            if (status != BGWH_NOT_YET_STARTED)
> +                break;
> +            rc = WaitLatch(&MyProc->procLatch,
> +                           WL_LATCH_SET | WL_POSTMASTER_DEATH, 0);
> +            if (rc & WL_POSTMASTER_DEATH)
> +            {
> +                status = BGWH_POSTMASTER_DIED;
> +                break;
> +            }
> +            ResetLatch(&MyProc->procLatch);
> +        }
> +    }
> +    PG_CATCH();
> +    {
> +        set_latch_on_sigusr1 = false;
> +        PG_RE_THROW();
> +    }
> +    PG_END_TRY();
> +
> +    set_latch_on_sigusr1 = false;
> +    *pidp = pid;
> +    return status;
> +}

Shouldn't that loop have a CHECK_FOR_INTERRUPTS()?

Theoretically this would unset set_latch_on_sigusr1 if it was previously
already set to 'true'. If you feel defensive you could safe the previous
value...

So, besides those I don't see much left to be done. I haven't tested
EXEC_BACKEND, but I don't anything specific to that here.

Greetings,

Andres Freund

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