Regarding BGworkers

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

Regarding BGworkers

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

--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: Regarding BGworkers

Robert Haas
On Sun, Jul 28, 2013 at 1:26 AM, Amit kapila <[hidden email]> 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: Regarding BGworkers

Michael Paquier



On Fri, Aug 2, 2013 at 1:22 AM, Robert Haas <[hidden email]> wrote:
On Sun, Jul 28, 2013 at 1:26 AM, Amit kapila <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Regarding BGworkers

amitkapila

On Friday, August 02, 2013 4:19 AM Michael Paquier wrote:
On Fri, Aug 2, 2013 at 1:22 AM, Robert Haas <[hidden email]> wrote:
On Sun, Jul 28, 2013 at 1:26 AM, Amit kapila <[hidden email]> 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.



--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: Regarding BGworkers

Alvaro Herrera-9
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 <[hidden email]> wrote:
> >> On Sun, Jul 28, 2013 at 1:26 AM, Amit kapila <[hidden email]> 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: Regarding BGworkers

Michael Paquier
On Fri, Aug 2, 2013 at 1:40 PM, Alvaro Herrera <[hidden email]> 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

20130806_bgworker_refactor_master.patch (19K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Regarding BGworkers

Robert Haas
On Mon, Aug 5, 2013 at 9:20 PM, Michael Paquier
<[hidden email]> wrote:
> On Fri, Aug 2, 2013 at 1:40 PM, Alvaro Herrera <[hidden email]> 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: Regarding BGworkers

Alvaro Herrera-9
Robert Haas escribió:

> On Mon, Aug 5, 2013 at 9:20 PM, Michael Paquier
> <[hidden email]> wrote:
> > On Fri, Aug 2, 2013 at 1:40 PM, Alvaro Herrera <[hidden email]> 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: Regarding BGworkers

Michael Paquier
On Tue, Aug 13, 2013 at 11:59 PM, Alvaro Herrera
<[hidden email]> 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: Regarding BGworkers

Robert Haas
On Tue, Aug 13, 2013 at 8:07 PM, Michael Paquier
<[hidden email]> wrote:
> On Tue, Aug 13, 2013 at 11:59 PM, Alvaro Herrera
> <[hidden email]> 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: Regarding BGworkers

Michael Paquier
On Wed, Aug 14, 2013 at 10:10 AM, Robert Haas <[hidden email]> 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

20130814_bgworker_refactor_93_v2.patch (4K) Download Attachment
20130814_bgworker_refactor_master_v2.patch (21K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Regarding BGworkers

Robert Haas
On Wed, Aug 14, 2013 at 8:04 AM, Michael Paquier
<[hidden email]> wrote:
> On Wed, Aug 14, 2013 at 10:10 AM, Robert Haas <[hidden email]> 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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers