Re: Dependency between bgw_notify_pid and bgw_flags

Ashutosh Bapat wrote:

> 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.

No kidding.

> Even tougher to add a parent auxilliary process, which spawns multiple
> worker processes.

I'm not sure about this point. The autovacuum launcher, which is an
obvious candidate to have "children" processes, does not do that but
instead talks to postmaster to do it instead. We did it that way to
avoid the danger of children processes connected to shared memory that
would not be direct children of postmaster; that could cause reliability
problems if the intermediate process fails to signal its children for
some reason. Along the same lines I would suggest that other bgworker
processes should also be controllers only, that is it can ask postmaster
to start other processes but not start them directly.

> 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.

If you want to make aux processes use the bgworker infrastructure (a
worthy goal I think), it is possible that more uses would be found for
those flags. For instance, avlauncher is equivalent to a worker that
has SHMEM_ACCESS but no DATABASE_CONNECTION; maybe some of the code
would differ depending on whether or not DATABASE_CONNECTION is
specified. The parallel to the avlauncher was the main reason I decided
to separate those two flags. Therefore I wouldn't try to simplify just
yet. If you succeed in making the avlauncher (and more generally all of
autovacuum) use bgworker code, perhaps that would be the time to search
for simplifications to make, because we would know more about how it
would be used.

From your list above it doesn't sound like DATABASE_CONNECTION actually
does anything useful other than sanity checks. I wonder if the behavior
that avlauncher becomes part of the backend list would be sane (this is
what would happen if you simply remove the DATABASE_CONNECTION flag and
then turn avlauncher into a regular bgworker).

On further thought, given that almost every aux process has particular
timing needs for start/stop under different conditions, I am doubtful
that you could turn any of them into a bgworker. Maybe pgstat would be
the only exception, perhaps bgwriter, not sure.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services