Обсуждение: Don't wake up to check trigger file if none is configured
A streaming replica waiting on WAL from the master will wake up every 5 seconds to check for a trigger file. This is pointless if no trigger file has been configured.
The attached patch suppresses the timeout when there is no trigger file configured.
A minor thing to be sure, but there was a campaign a couple years ago to remove such spurious wake-ups, so maybe this change is worthwhile.
I noticed that the existing codebase does not have a consensus on what to pass to WaitLatch for the timeout when the timeout isn't relevant. I picked 0, but -1L also has precedent.
Cheers,
Jeff
Вложения
On Sat, Nov 24, 2018 at 11:29 AM Jeff Janes <jeff.janes@gmail.com> wrote:
A streaming replica waiting on WAL from the master will wake up every 5 seconds to check for a trigger file. This is pointless if no trigger file has been configured.The attached patch suppresses the timeout when there is no trigger file configured.
Rebased over the removal of recovery.conf and renaming of TriggerFile.
If the promote_trigger_file param is someday made responsive to SIGHUP, I think this will just continue to do the right thing without further modification.
Cheers,
Jeff
Вложения
On Sat, Nov 24, 2018 at 11:29:22AM -0500, Jeff Janes wrote: > I noticed that the existing codebase does not have a consensus on what to > pass to WaitLatch for the timeout when the timeout isn't relevant. I picked > 0, but -1L also has precedent. WaitLatch enforces the timeout to -1 if WL_TIMEOUT is not given to the caller, so setting a value does not matter much. If WL_TIMEOUT is defined, the code would blow up on an assertion if the timeout is negative. In short I would let the timeout be set to 5000L, but just add the flag on the fly instead of having a if/else with two calls of WaitLatch() as your patch does. "git diff --check" complains about a whitespace issue. Anyway, the timeout is useful to have for another thing: we check if the WAL receiver is still alive thanks to that periodically, and this even if no promote file is defined. That's useful for failure handling if the WAL receiver gets killed so as the startup process can define more quickly a new source (or just create a new WAL sender) it should use for feeding with fresh WAL data, no? It seems to me that the comment on top of WaitLatch should be clearer about that, and that the current state leads to confusion. Another thing coming to my mind is that I think it would be useful to make the timeout configurable so as instances can react more quickly in the case of a sudden death of the WAL receiver (or to check faster for a trigger file if the HA application is to lazy to send a signal to the standby host). Attached is a patch to improve the comment for now. Thoughts? -- Michael
Вложения
On Wed, Dec 26, 2018 at 03:24:01PM +0900, Michael Paquier wrote: > It seems to me that the comment on top of WaitLatch should be clearer > about that, and that the current state leads to confusion. Another > thing coming to my mind is that I think it would be useful to make the > timeout configurable so as instances can react more quickly in the > case of a sudden death of the WAL receiver (or to check faster for a > trigger file if the HA application is to lazy to send a signal to the > standby host). > > Attached is a patch to improve the comment for now. So, does somebody have an objection if I apply the comment patch? Per the reasons above, the proposed patch is not correct, but the code can be more descriptive. -- Michael
Вложения
On Thu, Jan 31, 2019 at 04:09:56PM +0900, Michael Paquier wrote: > So, does somebody have an objection if I apply the comment patch? Per > the reasons above, the proposed patch is not correct, but the code can > be more descriptive. The comment patch has been applied, and the patch has been marked as returned with feedback. -- Michael