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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Jul 28, 2013 at 1:26 AM, Amit kapila <amit.kapila@huawei.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Aug 2, 2013 at 1:22 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Sun, Jul 28, 2013 at 1:26 AM, Amit kapila <amit.kapila@huawei.com>
wrote:2. Shouldn't function
do_start_bgworker()/StartOneBackgroundWorker(void) be moved to bgworker.c
as similar functions AutoVacWorkerMain()/PgArchiverMain() are intheir 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@gmail.com> wrote:
On Sun, Jul 28, 2013 at 1:26 AM, Amit kapila <amit.kapila@huawei.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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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@gmail.com> wrote:
On Sun, Jul 28, 2013 at 1:26 AM, Amit kapila <amit.kapila@huawei.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Aug 2, 2013 at 1:40 PM, Alvaro Herrera <alvherre@2ndquadrant.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
Attachments:
20130806_bgworker_refactor_master.patchapplication/octet-stream; name=20130806_bgworker_refactor_master.patchDownload+224-205
On Mon, Aug 5, 2013 at 9:20 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Fri, Aug 2, 2013 at 1:40 PM, Alvaro Herrera <alvherre@2ndquadrant.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas escribi�:
On Mon, Aug 5, 2013 at 9:20 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Fri, Aug 2, 2013 at 1:40 PM, Alvaro Herrera <alvherre@2ndquadrant.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Aug 13, 2013 at 11:59 PM, Alvaro Herrera
<alvherre@2ndquadrant.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Aug 13, 2013 at 8:07 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Tue, Aug 13, 2013 at 11:59 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:maybe_start_bgworker() in postmaster.c
do_start_bgworker() in postmaster.c
StartBackgroundWorker() in bgworker.cThis 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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Aug 14, 2013 at 10:10 AM, Robert Haas <robertmhaas@gmail.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.paquier@gmail.com> wrote:
On Wed, Aug 14, 2013 at 10:10 AM, Robert Haas <robertmhaas@gmail.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers