Re: Dependency between bgw_notify_pid and bgw_flags

On Sat, Aug 8, 2015 at 7:46 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Wed, Aug 5, 2015 at 3:33 AM, Ashutosh Bapat
> <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
> > This idea looks good.
>
> Thanks. It needs testing though to see if it really works as
> intended. Can you look into that?
>

PFA the patch containing your code changes + test module. See if that meets
your expectations.

>
> > 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).
>
> That's a separate patch, but, sure, we could do that. I agree with
> Alvaro's comments: the postmaster should start all children. Other
> processes should just request that it do so. We have two mechanisms
> for that right now: the one used by bgworkers, and the one used by the
> AV launcher.
>

BY children I really meant workers that it requests postmaster to start,
not the OS definition of child.

>
> > 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()
>
> There isn't, but you shouldn't do that. :-)
>
> This is C code; you can't protect against actively malicious code.
>

We have taken pains to check whether the worker was started with
BGWORKER_BACKEND_DATABASE_CONNECTION flag, when it requests to connect to a
database. I think it makes sense to do that with ACCESS_SHMEM flag as well.
Otherwise, some buggy extension would connect to the shared memory and exit
without postmaster restarting all the backends. Obvious one can argue that,
memory corruption is possible even without this flag, but we should try to
protect exposed interfaces.

>
> > 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.
>
> I think it's useful to support workers that don't have shared memory
> access at all, because those can crash without causing a system-wide
> reset. But I don't see the point in distinguishing between workers
> with shared-memory access and those with a database connection. I
> mean, obviously the worker needs to be able to initialize itself
> either way, but there seems to be no reason to force that to be
> signalled in bgw_flags. It can just depend on whether
> BackgroundWorkerInitializeConnection gets called.
>

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