Re: Dependency between bgw_notify_pid and bgw_flags

On Wed, Aug 5, 2015 at 2:07 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Tue, Jul 7, 2015 at 4:34 AM, Ashutosh Bapat
> <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
> > With that notion of backend, to fix the original problem I reported,
> > PostmasterMarkPIDForWorkerNotify() should also look at the
> > BackgroundWorkerList. As per the comments in the prologue of this
> function,
> > it assumes that only backends need to be notified about worker state
> change,
> > which looks too restrictive. Any backend or background worker, which
> wants
> > to be notified about a background worker it requested to be spawned,
> should
> > be allowed to do so.
>
> Yeah. I'm wondering if we should fix this problem by just insisting
> that all workers have an entry in BackendList. At first look, this
> seems like it would make the code a lot simpler and have virtually no
> downside. As it is, we're repeatedly reinventing new and different
> ways for unconnected background workers to do things that work fine in
> all other cases, and I don't see the point of that.
>
> I haven't really tested the attached patch - sadly, we have no testing
> of background workers without shared memory access in the tree - but
> it shows what I have in mind.
>
> Thoughts?
>
>
This idea looks good.

Looking at larger picture, we should also enable this feature to be used by
auxilliary processes. It's very hard to add a new auxilliary process in
current code. One has to go add code at many places to make sure that the
auxilliary processes die and are re-started correctly. Even tougher to add
a parent auxilliary process, which spawns multiple worker processes.That
would be whole lot simpler if we could allow the auxilliary processes to
use background worker infrastructure (which is what they are utlimately).

About the flags BGWORKER_BACKEND_DATABASE_CONNECTION and
BGWORKER_SHMEM_ACCESS
BGWORKER_BACKEND_DATABASE_CONNECTION is used at seven places in the code:
one is assertion, two check existence of this flag, when backend actually
connects to a database, fourth checks whether BGWORKER_SHMEM_ACCESS is also
set, fifth creates parallel workers with this flag, sixth uses the flag to
add backend to the backed list (which you have removed). Seventh usage is
only usage which installs signal handlers based on this flag, which too I
guess can be overridden (or set correctly) by the actual background worker
code.

BGWORKER_SHMEM_ACCESS has similar usage, except that it resets the on exit
callbacks and detaches the shared memory segment from the background
worker. That avoids a full cluster restart when one of those worker which
can not corrupt shared memory dies. But I do not see any check to prevent
such backend from calling PGSharedMemoryReattach()

So it looks like, it suffices to assume that background worker either needs
to access shared memory or doesn't. Any background worker having shared
memory access can also access database and thus becomes part of the backend
list. Or may be we just avoid these flags and treat every background worker
as if it passed both these flags. That will simplify a lot of code.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company