Re: Dependency between bgw_notify_pid and bgw_flags
| От | Ashutosh Bapat |
|---|---|
| Тема | Re: Dependency between bgw_notify_pid and bgw_flags |
| Дата | |
| Msg-id | CAFjFpReOdkooAin9t63DhRRYMg7pF=G86kGmwMDqU0_7qb8BfQ@mail.gmail.com обсуждение исходный текст |
| Ответ на | Re: Dependency between bgw_notify_pid and bgw_flags (Robert Haas <robertmhaas@gmail.com>) |
| Ответы |
Re: Dependency between bgw_notify_pid and bgw_flags
(Robert Haas <robertmhaas@gmail.com>)
|
| Список | pgsql-hackers |
In CleanupBackgroundWorker(), we seem to differentiate between a background worker with shared memory access and a backend.
2914 /*
2915 * Additionally, for shared-memory-connected workers, just like a
2916 * backend, any exit status other than 0 or 1 is considered a crash
2917 * and causes a system-wide restart.
2918 */
There is an assertion on line 2943 which implies that a backend should have a database connection.2914 /*
2915 * Additionally, for shared-memory-connected workers, just like a
2916 * backend, any exit status other than 0 or 1 is considered a crash
2917 * and causes a system-wide restart.
2918 */
2939
2940 /* Get it out of the BackendList and clear out remaining data */
2941 if (rw->rw_backend)
2942 {
2943 Assert(rw->rw_worker.bgw_flags & BGWORKER_BACKEND_DATABASE_CONNECTION);
A backend is a process created to handle a client connection [1], which is always connected to a database. After custom background workers were introduced we seem to have continued that notion. Hence only the background workers which request database connection are in BackendList. So, may be we should continue with that notion and correct the comment as "Background workers that request database connection during registration are in this list, too." or altogether delete that comment.
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.
5655 /*
5656 * When a backend asks to be notified about worker state changes, we
5657 * set a flag in its backend entry. The background worker machinery needs
5658 * to know when such backends exit.
5659 */
5660 bool
5661 PostmasterMarkPIDForWorkerNotify(int pid)
PFA the patch which changes PostmasterMarkPIDForWorkerNotify to look at BackgroundWorkerList as well.
The backends that request to be notified are marked using bgworker_notify, so that when such a backend dies the notifications to it can be turned off using BackgroundWorkerStopNotifications(). Now that we allow a background worker to be notified, we have to build similar flag in RegisteredBgWorker for the same purpose. The patch does that.
[1]. http://www.postgresql.org/developer/backend/
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.
5655 /*
5656 * When a backend asks to be notified about worker state changes, we
5657 * set a flag in its backend entry. The background worker machinery needs
5658 * to know when such backends exit.
5659 */
5660 bool
5661 PostmasterMarkPIDForWorkerNotify(int pid)
PFA the patch which changes PostmasterMarkPIDForWorkerNotify to look at BackgroundWorkerList as well.
The backends that request to be notified are marked using bgworker_notify, so that when such a backend dies the notifications to it can be turned off using BackgroundWorkerStopNotifications(). Now that we allow a background worker to be notified, we have to build similar flag in RegisteredBgWorker for the same purpose. The patch does that.
[1]. http://www.postgresql.org/developer/backend/
On Mon, Jun 8, 2015 at 11:22 PM, Robert Haas <robertmhaas@gmail.com> wrote:
Well, it sounds like we at least need to think about making itOn Mon, Jun 8, 2015 at 1:51 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Robert Haas wrote:
>
>> After studying this, I think it's a bug. See this comment:
>>
>> * Normal child backends can only be launched when we are in PM_RUN or
>> * PM_HOT_STANDBY state. (We also allow launch of normal
>> * child backends in PM_WAIT_BACKUP state, but only for superusers.)
>> * In other states we handle connection requests by launching "dead_end"
>> * child processes, which will simply send the client an error message and
>> * quit. (We track these in the BackendList so that we can know when they
>> * are all gone; this is important because they're still connected to shared
>> * memory, and would interfere with an attempt to destroy the shmem segment,
>> * possibly leading to SHMALL failure when we try to make a new one.)
>> * In PM_WAIT_DEAD_END state we are waiting for all the dead_end children
>> * to drain out of the system, and therefore stop accepting connection
>> * requests at all until the last existing child has quit (which hopefully
>> * will not be very long).
>>
>> That comment seems to imply that, at the very least, all backends with
>> shared-memory access need to be part of BackendList. But really, I'm
>> unclear why we'd ever want to exit without all background workers
>> having shut down, so maybe they should all be in there. Isn't that
>> sorta the point of this facility?
>>
>> Alvaro, any thoughts?
>
> As I recall, my thinking was:
>
> * anything connected to shmem that crashes could leave things in bad
> state (for example locks improperly held), whereas things not connected
> to shmem could crash without it being a problem for the rest of the
> system and thus do not require a full restart cycle. This stuff is
> detected with the PMChildSlot thingy; therefore all bgworkers with shmem
> access should have one of these.
>
> * I don't recall offhand what the criteria is for stuff to be in
> postmaster's BackendList. According to the comment on top of struct
> Backend, bgworkers connected to shmem should be on that list, even if
> they did not have the BGWORKER_BACKEND_DATABASE_CONNECTION flag on
> registration. So that would be a bug.
>
> Does this help you any?
consistently depend on shmem-access rather than database-connection.
I'm less sure what to do with workers that have not even got shmem
access.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Вложения
В списке pgsql-hackers по дате отправления: