On Thu, Nov 17, 2022 at 12:21 AM Robert Haas <robertmh...@gmail.com> wrote: > > On Wed, Nov 16, 2022 at 1:47 AM Bharath Rupireddy > <bharath.rupireddyforpostg...@gmail.com> wrote: > > That can be done, only if we can disable the timeout in another place > > when the StandbyMode is set to true in ReadRecord(), that is, after > > the standby server finishes crash recovery and enters standby mode. > > Oh, interesting. I didn't realize that we would need to worry about that case. > > > I'm attaching the v3 patch for further review. Please find the CF > > entry here - https://commitfest.postgresql.org/41/4012/. > > I kind of dislike having to have logic for this in two places. Seems > like it could create future bugs.
Duplication is a problem that I agree with and I have an idea here - how about introducing a new function, say EnableStandbyMode() that sets StandbyMode to true and disables the startup progress timeout, something like the attached? > How about the attached approach, instead? This way, the first time the > timer expires after we reach standby mode, we reactively disable it. Hm. I'm not really sure if it's a good idea. While it simplifies the code, the has_startup_progress_timeout_expired() gets called for every WAL record in standby mode. Isn't this an unnecessary thing? Currently, the if (!StandbyMode) condition blocks the function calls. And I'm also a little concerned that we move the StandbyMode variable to startup.c which so far tiled to xlogrecovery.c. Maybe these are not really concerns at all. Maybe others are okay with this approach. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
From b25838215534488c53855a5cd5009fba09bd358a Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> Date: Thu, 17 Nov 2022 07:13:27 +0000 Subject: [PATCH v4] Disable STARTUP_PROGRESS_TIMEOUT in standby mode In standby mode, we actually don't report progress of recovery to not bloat server logs as the standby is always in recovery unless promoted. However, startup_progress_timeout_handler() gets called every log_startup_progress_interval seconds, which is unnecessary. Therefore, we disable the timeout in standby mode. Reported-by: Thomas Munro Authors: Bharath Rupireddy, Simon Riggs Reviewed-by: Robert Haas Backpatch-through: 15 Discussion: https://www.postgresql.org/message-id/CA%2BhUKGKCHSffAj8zZJKJvNX7ygnQFxVD6wm1d-2j3fVw%2BMafPQ%40mail.gmail.com --- src/backend/access/transam/xlogrecovery.c | 21 +++++++++++++--- src/backend/postmaster/startup.c | 29 ++++++++++++++++++++--- src/include/postmaster/startup.h | 2 ++ 3 files changed, 46 insertions(+), 6 deletions(-) diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c index cb07694aea..2feae1ebd3 100644 --- a/src/backend/access/transam/xlogrecovery.c +++ b/src/backend/access/transam/xlogrecovery.c @@ -386,6 +386,7 @@ static bool recoveryStopAfter; /* prototypes for local functions */ static void ApplyWalRecord(XLogReaderState *xlogreader, XLogRecord *record, TimeLineID *replayTLI); +static void EnableStandbyMode(void); static void readRecoverySignalFile(void); static void validateRecoveryParameters(void); static bool read_backup_label(XLogRecPtr *checkPointLoc, @@ -470,6 +471,20 @@ XLogRecoveryShmemInit(void) ConditionVariableInit(&XLogRecoveryCtl->recoveryNotPausedCV); } +static void +EnableStandbyMode(void) +{ + StandbyMode = true; + + /* + * To avoid server log bloat, we don't report recovery progress in a + * standby as it will always be in recovery unless promoted. We disable + * startup progress timeout in standby mode to avoid calling + * startup_progress_timeout_handler() unnecessarily. + */ + disable_startup_progress_timeout(); +} + /* * Prepare the system for WAL recovery, if needed. * @@ -603,7 +618,7 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr, */ InArchiveRecovery = true; if (StandbyModeRequested) - StandbyMode = true; + EnableStandbyMode(); /* * When a backup_label file is present, we want to roll forward from @@ -740,7 +755,7 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr, { InArchiveRecovery = true; if (StandbyModeRequested) - StandbyMode = true; + EnableStandbyMode(); } /* Get the last valid checkpoint record. */ @@ -3115,7 +3130,7 @@ ReadRecord(XLogPrefetcher *xlogprefetcher, int emode, (errmsg_internal("reached end of WAL in pg_wal, entering archive recovery"))); InArchiveRecovery = true; if (StandbyModeRequested) - StandbyMode = true; + EnableStandbyMode(); SwitchIntoArchiveRecovery(xlogreader->EndRecPtr, replayTLI); minRecoveryPoint = xlogreader->EndRecPtr; diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c index f99186eab7..2705e425e8 100644 --- a/src/backend/postmaster/startup.c +++ b/src/backend/postmaster/startup.c @@ -314,11 +314,22 @@ startup_progress_timeout_handler(void) startup_progress_timer_expired = true; } +void +disable_startup_progress_timeout(void) +{ + /* Feature is disabled. */ + if (log_startup_progress_interval == 0) + return; + + disable_timeout(STARTUP_PROGRESS_TIMEOUT, false); + startup_progress_timer_expired = false; +} + /* * Set the start timestamp of the current operation and enable the timeout. */ void -begin_startup_progress_phase(void) +enable_startup_progress_timeout(void) { TimestampTz fin_time; @@ -326,8 +337,6 @@ begin_startup_progress_phase(void) if (log_startup_progress_interval == 0) return; - disable_timeout(STARTUP_PROGRESS_TIMEOUT, false); - startup_progress_timer_expired = false; startup_progress_phase_start_time = GetCurrentTimestamp(); fin_time = TimestampTzPlusMilliseconds(startup_progress_phase_start_time, log_startup_progress_interval); @@ -335,6 +344,20 @@ begin_startup_progress_phase(void) log_startup_progress_interval); } +/* + * A thin wrapper to first disable and then enable the startup progress timeout. + */ +void +begin_startup_progress_phase(void) +{ + /* Feature is disabled. */ + if (log_startup_progress_interval == 0) + return; + + disable_startup_progress_timeout(); + enable_startup_progress_timeout(); +} + /* * Report whether startup progress timeout has occurred. Reset the timer flag * if it did, set the elapsed time to the out parameters and return true, diff --git a/src/include/postmaster/startup.h b/src/include/postmaster/startup.h index d66ec1fcb1..12897531c3 100644 --- a/src/include/postmaster/startup.h +++ b/src/include/postmaster/startup.h @@ -32,6 +32,8 @@ extern void PostRestoreCommand(void); extern bool IsPromoteSignaled(void); extern void ResetPromoteSignaled(void); +extern void enable_startup_progress_timeout(void); +extern void disable_startup_progress_timeout(void); extern void begin_startup_progress_phase(void); extern void startup_progress_timeout_handler(void); extern bool has_startup_progress_timeout_expired(long *secs, int *usecs); -- 2.34.1
