Обсуждение: fix worker_spi to run as non-dynamic background worker
Hi all, I noticed that contrib/worker_spi can't run as non-dynamic background worker (IOW, load via shared_preload_libraries), because of uninitialized bgw_notify_pid. I got log lines below when starting PostgreSQL with shared_preload_libraries = 'worker_spi'. $ pg_ctl start -w waiting for server to start....LOG: registering background worker "worker 1" LOG: background worker "worker 1": only dynamic background workers can request notification LOG: registering background worker "worker 2" LOG: background worker "worker 2": only dynamic background workers can request notification LOG: redirecting log output to logging collector process HINT: Future log output will appear in directory "pg_log". Attached patch fixes this issue. Please apply onto HEAD and 9.4. -- Shigeru HANADA
Вложения
On Fri, May 23, 2014 at 6:29 PM, Shigeru Hanada <shigeru.hanada@gmail.com> wrote: > I noticed that contrib/worker_spi can't run as non-dynamic background > worker (IOW, load via shared_preload_libraries), because of > uninitialized bgw_notify_pid. That's actually the case of all the bgworkers that have been developed with 9.3 and not only worker_spi. If bgw_notify_pid needs to be specifically initialized with 9.4, this should be mentioned in the release notes or users will be surprised. -- Michael
On Fri, May 23, 2014 at 5:29 AM, Shigeru Hanada <shigeru.hanada@gmail.com> wrote: > I noticed that contrib/worker_spi can't run as non-dynamic background > worker (IOW, load via shared_preload_libraries), because of > uninitialized bgw_notify_pid. > > Attached patch fixes this issue. Please apply onto HEAD and 9.4. Good catch. Patch committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, May 23, 2014 at 10:38 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Fri, May 23, 2014 at 6:29 PM, Shigeru Hanada > <shigeru.hanada@gmail.com> wrote: >> I noticed that contrib/worker_spi can't run as non-dynamic background >> worker (IOW, load via shared_preload_libraries), because of >> uninitialized bgw_notify_pid. > That's actually the case of all the bgworkers that have been developed > with 9.3 and not only worker_spi. If bgw_notify_pid needs to be > specifically initialized with 9.4, this should be mentioned in the > release notes or users will be surprised. I agree that it would be appropriate to mention this in the release notes, maybe in the incompatibilities section. The basic idea is: When registering a background worker, the new bgw_notify_pid field should be initialized to zero, except if you're using the new notification feature. That feature (introduced in commit 090d0f2050647958865cb495dff74af7257d2bb4) doesn't seem to really be mentioned in the release notes at present, so we might want to add that too. It doesn't necessarily have to be separate from the existing background worker item, although it could be, but it should be mentioned somehow, particularly because of the related incompatibility. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, May 27, 2014 at 12:09 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Fri, May 23, 2014 at 10:38 AM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Fri, May 23, 2014 at 6:29 PM, Shigeru Hanada >> <shigeru.hanada@gmail.com> wrote: >>> I noticed that contrib/worker_spi can't run as non-dynamic background >>> worker (IOW, load via shared_preload_libraries), because of >>> uninitialized bgw_notify_pid. >> That's actually the case of all the bgworkers that have been developed >> with 9.3 and not only worker_spi. If bgw_notify_pid needs to be >> specifically initialized with 9.4, this should be mentioned in the >> release notes or users will be surprised. > > I agree that it would be appropriate to mention this in the release > notes, maybe in the incompatibilities section. The basic idea is: > When registering a background worker, the new bgw_notify_pid field > should be initialized to zero, except if you're using the new > notification feature. This is an item for the section "Migration to 9.4" IMO. > That feature (introduced in commit > 090d0f2050647958865cb495dff74af7257d2bb4) doesn't seem to really be > mentioned in the release notes at present, so we might want to add > that too. It doesn't necessarily have to be separate from the > existing background worker item, although it could be, but it should > be mentioned somehow, particularly because of the related > incompatibility. The feature itself is already mentioned in the release notes in a very general manner, by telling that bgworkers can be dynamically "registered" and "started". Users can still refer to the documentation to see more precisely what kind of infrastructure is in place in 9.4. So... As I'm on it... What about something like the attached patch? -- Michael
Вложения
On Tue, May 27, 2014 at 12:11 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > The feature itself is already mentioned in the release notes in a very > general manner, by telling that bgworkers can be dynamically > "registered" and "started". Users can still refer to the documentation > to see more precisely what kind of infrastructure is in place in 9.4. > > So... As I'm on it... What about something like the attached patch? Hearing no comment from Bruce, committed. Bruce, feel free to modify or revert if you don't think this is appropriate. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company