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 CAFjFpRdU4hFRSYDTx=bGw0OYb0QFHfpUwAEjROY_mhoLbP-ZZQ@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  (Robert Haas <robertmhaas@gmail.com>)
List pgsql-hackers


On Sat, Aug 8, 2015 at 7:46 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Aug 5, 2015 at 3:33 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.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
Attachment

pgsql-hackers by date:

Previous
From: Amit Kapila
Date:
Subject: Re: Reducing ClogControlLock contention
Next
From: Magnus Hagander
Date:
Subject: Re: Information of pg_stat_ssl visible to all users