On Wed, Aug 5, 2015 at 2:07 AM, Robert Haas <robertmh...@gmail.com> wrote:
> On Tue, Jul 7, 2015 at 4:34 AM, Ashutosh Bapat > <ashutosh.ba...@enterprisedb.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
