Thread: reaper should restart archiver even on standby
Hi, When the archiver exits, currently reaper() restarts it only while the postmaster state is PM_RUN. This is OK in 9.4 or before because the archiver could be running on that state. But in 9.5, we can set archive_mode to "always" and start the archiver even on the standby. So I think that reaper() should restart the archiver even when the postmaster state is PM_RECOVERY or PM_HOT_STANDBY with some conditions. Patch attached. Regards, -- Fujii Masao
Attachment
Fujii Masao wrote: > Hi, > > When the archiver exits, currently reaper() restarts it only while > the postmaster state is PM_RUN. This is OK in 9.4 or before because > the archiver could be running on that state. But in 9.5, we can set > archive_mode to "always" and start the archiver even on the standby. > So I think that reaper() should restart the archiver even when > the postmaster state is PM_RECOVERY or PM_HOT_STANDBY with > some conditions. Patch attached. Sounds reasonable, but the patch looks pretty messy. Can't we create some common function that would be called both here and on ServerLoop? We also have sigusr1_handler that starts an archiver -- why does that one use different conditions? Also, the block at lines 2807ff seems like it ought to be changed as well? -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Jun 9, 2015 at 5:21 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Fujii Masao wrote: >> Hi, >> >> When the archiver exits, currently reaper() restarts it only while >> the postmaster state is PM_RUN. This is OK in 9.4 or before because >> the archiver could be running on that state. But in 9.5, we can set >> archive_mode to "always" and start the archiver even on the standby. >> So I think that reaper() should restart the archiver even when >> the postmaster state is PM_RECOVERY or PM_HOT_STANDBY with >> some conditions. Patch attached. > > Sounds reasonable, but the patch looks pretty messy. Thanks for reviewing the patch! > Can't we create > some common function that would be called both here and on ServerLoop? Agreed. So, what about the attached patch? > We also have sigusr1_handler that starts an archiver -- why does that > one use different conditions? Because that code path can be reached only during recovery. So XLogArchivingActive() which indicates whether archiver is allowed to start during normal processing doesn't need to be checked there. OTOH, in the other places where archiver is started up, we can reach there during not only recovery but also normal processing. So the conditions that we need to check are different. Regards, -- Fujii Masao
Fujii Masao wrote: > On Tue, Jun 9, 2015 at 5:21 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > Fujii Masao wrote: > > Can't we create > > some common function that would be called both here and on ServerLoop? > > Agreed. So, what about the attached patch? No attachment ... > > We also have sigusr1_handler that starts an archiver -- why does that > > one use different conditions? > > Because that code path can be reached only during recovery. > So XLogArchivingActive() which indicates whether archiver is > allowed to start during normal processing doesn't need to be > checked there. Makes sense. > OTOH, in the other places where archiver is started up, > we can reach there during not only recovery but also normal processing. > So the conditions that we need to check are different. I think it would be simpler to centralize knowledge in a single function, and have that function take an argument indicating whether we're in recovery or normal processing, instead of spreading it to every place that can possibly start the archiver. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Jun 10, 2015 at 11:12 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Fujii Masao wrote: >> On Tue, Jun 9, 2015 at 5:21 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: >> > Fujii Masao wrote: > >> > Can't we create >> > some common function that would be called both here and on ServerLoop? >> >> Agreed. So, what about the attached patch? > > No attachment ... Ooops... Attached. >> OTOH, in the other places where archiver is started up, >> we can reach there during not only recovery but also normal processing. >> So the conditions that we need to check are different. > > I think it would be simpler to centralize knowledge in a single > function, and have that function take an argument indicating whether > we're in recovery or normal processing, instead of spreading it to every > place that can possibly start the archiver. Agreed. The attached patch defines the macro to check whether archiver is allowed to start up or not, and uses it everywhere except sigusr1_handler. I made sigusr1_handler use a different condition because only it tries to start archiver in PM_STARTUP postmaster state and it looks a bit messy to add the check of that state into the centralized check condition. Regards, -- Fujii Masao
Attachment
Fujii Masao wrote: > Agreed. The attached patch defines the macro to check whether archiver is > allowed to start up or not, and uses it everywhere except sigusr1_handler. > I made sigusr1_handler use a different condition because only it tries to > start archiver in PM_STARTUP postmaster state and it looks a bit messy > to add the check of that state into the centralized check condition. WFM, but do these macros in xlog.h need a one-line comment to state their purpose? -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Jun 11, 2015 at 1:39 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Fujii Masao wrote: > >> Agreed. The attached patch defines the macro to check whether archiver is >> allowed to start up or not, and uses it everywhere except sigusr1_handler. >> I made sigusr1_handler use a different condition because only it tries to >> start archiver in PM_STARTUP postmaster state and it looks a bit messy >> to add the check of that state into the centralized check condition. > > WFM, but do these macros in xlog.h need a one-line comment to state > their purpose? Yes, I added the comments and just pushed the patch. Thanks! Regards, -- Fujii Masao