Re: Dependency between bgw_notify_pid and bgw_flags - Mailing list pgsql-hackers
| From | Ashutosh Bapat |
|---|---|
| Subject | Re: Dependency between bgw_notify_pid and bgw_flags |
| Date | |
| Msg-id | CAFjFpReOmikLCaW2T_yY9w+TK82FKo=ueYgDPaRdiF=Pu5suhw@mail.gmail.com Whole thread Raw |
| In response to | Re: Dependency between bgw_notify_pid and bgw_flags (Robert Haas <robertmhaas@gmail.com>) |
| Responses |
Re: Dependency between bgw_notify_pid and bgw_flags
(Alvaro Herrera <alvherre@2ndquadrant.com>)
Re: Dependency between bgw_notify_pid and bgw_flags (Robert Haas <robertmhaas@gmail.com>) |
| List | pgsql-hackers |
On Wed, Aug 5, 2015 at 2:07 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Jul 7, 2015 at 4:34 AM, Ashutosh Bapat
<ashutosh.bapat@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()
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
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
pgsql-hackers by date: