Lists: | pgsql-hackers |
---|
From: | Amit kapila <amit(dot)kapila(at)huawei(dot)com> |
---|---|
To: | "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Regarding BGworkers |
Date: | 2013-07-28 05:26:23 |
Message-ID: | 6C0B27F7206C9E4CA54AE035729E9C38421C6E45@szxeml558-mbs.china.huawei.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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.
From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | Amit kapila <amit(dot)kapila(at)huawei(dot)com> |
Cc: | "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Regarding BGworkers |
Date: | 2013-08-01 16:22:05 |
Message-ID: | CA+TgmoaGgYGm=VepraJD5MmSEuaO=dYp=HWVhhqChMvX5_N9zQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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
From: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Amit kapila <amit(dot)kapila(at)huawei(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Regarding BGworkers |
Date: | 2013-08-01 22:48:37 |
Message-ID: | CAB7nPqT6oOVwWCaRGamXkfp1=_=o3joQrRyNmU94dXTF3KivJg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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
From: | Amit Kapila <amit(dot)kapila(at)huawei(dot)com> |
---|---|
To: | "'Michael Paquier'" <michael(dot)paquier(at)gmail(dot)com>, "'Robert Haas'" <robertmhaas(at)gmail(dot)com> |
Cc: | <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Regarding BGworkers |
Date: | 2013-08-02 04:35:36 |
Message-ID: | 007c01ce8f39$bc4cf130$34e6d390$@kapila@huawei.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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.
From: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> |
---|---|
To: | Amit Kapila <amit(dot)kapila(at)huawei(dot)com> |
Cc: | 'Michael Paquier' <michael(dot)paquier(at)gmail(dot)com>, 'Robert Haas' <robertmhaas(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: Regarding BGworkers |
Date: | 2013-08-02 04:40:16 |
Message-ID: | 20130802044016.GH5669@eldon.alvh.no-ip.org |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | 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(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
From: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
---|---|
To: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> |
Cc: | Amit Kapila <amit(dot)kapila(at)huawei(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Regarding BGworkers |
Date: | 2013-08-06 01:20:25 |
Message-ID: | CAB7nPqR=SPzPj5s0AhA-8Mt+WA4C4yv7B4i5mn0ZVk9Ev=BkVA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
20130806_bgworker_refactor_master.patch | application/octet-stream | 13.8 KB |
From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
Cc: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Regarding BGworkers |
Date: | 2013-08-13 11:30:35 |
Message-ID: | CA+TgmoYJVdSXkLfNxW0nb3_eEwL0Y9EpQfgkpgoP5kWyMTKbyg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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
From: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Regarding BGworkers |
Date: | 2013-08-13 14:59:43 |
Message-ID: | 20130813145943.GA8742@eldon.alvh.no-ip.org |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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
From: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
---|---|
To: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> |
Cc: | Robert Haas <robertmhaas(at)gmail(dot)com>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Regarding BGworkers |
Date: | 2013-08-14 00:07:06 |
Message-ID: | CAB7nPqSN1mi5bAmz6X-u9h11bbbwBzGDWbnhaW8DCc9P4VH_1w@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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
From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
Cc: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Regarding BGworkers |
Date: | 2013-08-14 01:10:11 |
Message-ID: | CA+TgmobTVe5QH5X=jyD1T1j4-XWJDgRtZo69UmnDttTvYGVFBw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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
From: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Regarding BGworkers |
Date: | 2013-08-14 12:04:06 |
Message-ID: | CAB7nPqTZzoc6qi=ziW=sFyj5mpAd_CYu+hqVAKTcZ0gVCq7Dyw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
20130814_bgworker_refactor_93_v2.patch | application/octet-stream | 3.4 KB |
20130814_bgworker_refactor_master_v2.patch | application/octet-stream | 15.9 KB |
From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
Cc: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Regarding BGworkers |
Date: | 2013-08-16 19:32:22 |
Message-ID: | CA+TgmoavJVc4K9xpMNkKU+g0rY6H2vssBo5E=VxfTaRihr=1qA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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