Re: dynamic background workers, round two

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(at)anarazel(dot)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