Thread: 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 workerlist into shared memory. Record the * shared memory slot assigned to each worker. This ensures * a 1-to-1 correspondencebetwen 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 needsto be updated for new functionality of dynamic workers. With Regards, Amit Kapila.
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
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:Yes, perhaps so. Other votes?
> 2. Shouldn't function
> do_start_bgworker()/StartOneBackgroundWorker(void) be moved to bgworker.c
> as similar functions AutoVacWorkerMain()/PgArchiverMain() are in their respective files.
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.
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
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
Attachment
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
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() inbgworker.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@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
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.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@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
Attachment
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