Re: Regarding BGworkers

While going through below commit, few doubts/observations:
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=7f7485a0cde92aa4ba235a1ffe4dda0ca0b6cc9a

1. Bgworker.c -
FindRegisteredWorkerBySlotNumber()
{
..
/*
* Copy contents of worker list into shared memory. Record the
* shared memory slot assigned to each worker. This ensures
* a 1-to-1 correspondence betwen the postmaster's private list and
* the array in shared memory.
*/
..
}
a. Comment in function doesn't seem to be appropriate. It seems copy-pasted from function
BackgroundWorkerShmemInit
b. all function's except this have function header to explain a bit about function, though
it might not be required here, but not sure so pointed.

2. Shouldn't function
do_start_bgworker()/StartOneBackgroundWorker(void) be moved to bgworker.c
as similar functions AutoVacWorkerMain()/PgArchiverMain() are in their respective files.

3. bgworker.h - file header still contains explanation only as per old functionality.
Not sure, if it needs to be updated for new functionality of dynamic workers.

With Regards,
Amit Kapila.

On Sun, Jul 28, 2013 at 1:26 AM, Amit kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
> 1. Bgworker.c -
> FindRegisteredWorkerBySlotNumber()
> {
> ..
> /*
> * Copy contents of worker list into shared memory. Record the
> * shared memory slot assigned to each worker. This ensures
> * a 1-to-1 correspondence betwen the postmaster's private list and
> * the array in shared memory.
> */
> ..
> }
> a. Comment in function doesn't seem to be appropriate. It seems copy-pasted from function
> BackgroundWorkerShmemInit
> b. all function's except this have function header to explain a bit about function, though
> it might not be required here, but not sure so pointed.

Fixed.

> 2. Shouldn't function
> do_start_bgworker()/StartOneBackgroundWorker(void) be moved to bgworker.c
> as similar functions AutoVacWorkerMain()/PgArchiverMain() are in their respective files.

Yes, perhaps so. Other votes?

> 3. bgworker.h - file header still contains explanation only as per old functionality.
> Not sure, if it needs to be updated for new functionality of dynamic workers.

Fixed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Aug 2, 2013 at 1:22 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Sun, Jul 28, 2013 at 1:26 AM, Amit kapila <amit(dot)kapila(at)huawei(dot)com>
> wrote:
> > 2. Shouldn't function
> > do_start_bgworker()/StartOneBackgroundWorker(void) be moved to bgworker.c
> > as similar functions AutoVacWorkerMain()/PgArchiverMain() are in
> their respective files.
>
> Yes, perhaps so. Other votes?
>
StartOneBackgroundWorker uses StartWorkerNeeded and HaveCrashedWorker, and
IMO, we should not expose that outside the postmaster. On the contrary,
moving do_start_bgworker would be fine, as it uses nothing exclusive to the
postmaster as far as I saw, and it would also make it more consistent with
the other features.

Regards,
--
Michael

On Friday, August 02, 2013 4:19 AM Michael Paquier wrote:
On Fri, Aug 2, 2013 at 1:22 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
On Sun, Jul 28, 2013 at 1:26 AM, Amit kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
>>> 2. Shouldn't function
>>> do_start_bgworker()/StartOneBackgroundWorker(void) be moved to
bgworker.c
>>>    as similar functions AutoVacWorkerMain()/PgArchiverMain() are in
their respective files.

>> Yes, perhaps so.  Other votes?

> StartOneBackgroundWorker uses StartWorkerNeeded and HaveCrashedWorker, and
IMO, we should not expose that outside the postmaster.

How about exposing Set/Get for these from bgworker?

> On the contrary,
> moving do_start_bgworker would be fine, as it uses nothing exclusive to
the postmaster as far as I saw, and it would also make it more consistent
with > the other features.

With Regards,
Amit Kapila.

Amit Kapila escribió:
>
> On Friday, August 02, 2013 4:19 AM Michael Paquier wrote:
> >On Fri, Aug 2, 2013 at 1:22 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> >> On Sun, Jul 28, 2013 at 1:26 AM, Amit kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
> >>> 2. Shouldn't function
> >>> do_start_bgworker()/StartOneBackgroundWorker(void) be moved to
> >>> bgworker.c
> >>>    as similar functions AutoVacWorkerMain()/PgArchiverMain() are in
> >>> their respective files.
>
> >> Yes, perhaps so.  Other votes?
>
> > StartOneBackgroundWorker uses StartWorkerNeeded and HaveCrashedWorker, and
> > IMO, we should not expose that outside the postmaster.
>
> How about exposing Set/Get for these from bgworker?

That seems more mess than just keeping that function in postmaster.c.
I agree with moving the other one.

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

On Fri, Aug 2, 2013 at 1:40 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> That seems more mess than just keeping that function in postmaster.c.
> I agree with moving the other one.
Please find attached a patch for that can be applied on master branch.
do_start_bgworker is renamed to StartBackgroundWorker and moved to
bgworker.c. At the same time, bgworker_quickdie, bgworker_die and
bgworker_sigusr1_handler are moved to bgworker.c as they are used in
do_start_bgworker.

Regards,
--
Michael

On Mon, Aug 5, 2013 at 9:20 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Fri, Aug 2, 2013 at 1:40 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>> That seems more mess than just keeping that function in postmaster.c.
>> I agree with moving the other one.
> Please find attached a patch for that can be applied on master branch.
> do_start_bgworker is renamed to StartBackgroundWorker and moved to
> bgworker.c. At the same time, bgworker_quickdie, bgworker_die and
> bgworker_sigusr1_handler are moved to bgworker.c as they are used in
> do_start_bgworker.

This particular formulation doesn't seem quite good to me, because
we'd end up with a function called StartBackgroundWorker() and another
called StartOneBackgroundWorker() doing related but different things.
Maybe we can name things a bit better?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas escribió:
> On Mon, Aug 5, 2013 at 9:20 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
> > On Fri, Aug 2, 2013 at 1:40 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> >> That seems more mess than just keeping that function in postmaster.c.
> >> I agree with moving the other one.
> > Please find attached a patch for that can be applied on master branch.
> > do_start_bgworker is renamed to StartBackgroundWorker and moved to
> > bgworker.c. At the same time, bgworker_quickdie, bgworker_die and
> > bgworker_sigusr1_handler are moved to bgworker.c as they are used in
> > do_start_bgworker.
>
> This particular formulation doesn't seem quite good to me, because
> we'd end up with a function called StartBackgroundWorker() and another
> called StartOneBackgroundWorker() doing related but different things.
> Maybe we can name things a bit better?

Yeah, we also have start_bgworker(). I agree that we should rename
things so that they make as much sense as possible.

In the current code, we have this:

StartOneBackgroundWorker() in postmaster.c
start_bgworker() in postmaster.c
do_start_bgworker() in postmaster.c

With this patch we would have
StartOneBackgroundWorker() in postmaster.c
start_bgworker() in postmaster.c
StartBackgroundWorker() in bgworker.c

I think we should rename to something like this:

maybe_start_bgworker() in postmaster.c
do_start_bgworker() in postmaster.c
StartBackgroundWorker() in bgworker.c

(I would also rename the functions in 9.3 to avoid inconsistency). Not
wedded to those particular names, but (1) I would add the "maybe" prefix
because that's what that function does; and (2) it seems to me that
stuff in bgworker.c tend to use CamelCaseNaming and postmaster.c uses
names_with_stuffed_underscores.

(My convention tends to be that "internal" stuff uses underscores while
exposed APIs use CamelCase. I probably fail to do it really
consistently.)

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

On Tue, Aug 13, 2013 at 11:59 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> maybe_start_bgworker() in postmaster.c
> do_start_bgworker() in postmaster.c
> StartBackgroundWorker() in bgworker.c
This formulation is fine, thanks. Instead of maybe_start_bgworker,
what about start_bgworker_if_necessary?
--
Michael

On Tue, Aug 13, 2013 at 8:07 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Tue, Aug 13, 2013 at 11:59 PM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)com> wrote:
>> maybe_start_bgworker() in postmaster.c
>> do_start_bgworker() in postmaster.c
>> StartBackgroundWorker() in bgworker.c
> This formulation is fine, thanks. Instead of maybe_start_bgworker,
> what about start_bgworker_if_necessary?

I think Alvaro's suggestion is better. It's shorter, and makes clear
that at most one will be started.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Aug 14, 2013 at 10:10 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I think Alvaro's suggestion is better. It's shorter, and makes clear
> that at most one will be started.
OK cool. Here are patches for 9.3 and master respecting those comments.

Regards,
--
Michael

On Wed, Aug 14, 2013 at 8:04 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Aug 14, 2013 at 10:10 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> I think Alvaro's suggestion is better. It's shorter, and makes clear
>> that at most one will be started.
> OK cool. Here are patches for 9.3 and master respecting those comments.

Thanks, committed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company