Re: dynamic background workers, round two

On Tue, Aug 27, 2013 at 9:50 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> > 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 ;)

Well, fair enough. But you might have to look around a bit to figure
out that you now have two functions each of which can return 3 out of
a possible 4 values. But whatever.

>> + <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/?

This seems like a possible doc clarification not intimately related to
the patch at hand. The only reason that paragraph is getting reflowed
is because of the additional argument to
RegisterDynamicBackgroundWorker(). On the substance, I could go
either way on whether your proposed text is better than what's there
now. It seems like it might need a bit of wordsmithing, anyway.

> s/STATED/STARTED/

Good catch.

>> 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...

I can add a check for that. I agree that it's a separate patch.

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

Sure.

> Shouldn't that loop have a CHECK_FOR_INTERRUPTS()?

Yep.

>
> 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...

Probably a good plan.

> 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.

I certainly can't promise that the code is bug-free. But I think it's
probably better to get this into the tree and let people start playing
around with it than to continue to maintain it in my private sandbox.
At this point it's just infrastructure anyway, though there seems to
be a good deal of interest in it from more than just myself... and I
do think it's a useful step forward from where we are today.

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