On Tue, Nov 15, 2022 at 10:55 PM Robert Haas <robertmh...@gmail.com> wrote: > > On Tue, Nov 15, 2022 at 8:33 AM Bharath Rupireddy > <bharath.rupireddyforpostg...@gmail.com> wrote: > > Please review the v2 patch. > > It seems to me that this will call disable_startup_progress_timeout > once per WAL record, which seems like an unnecessary expense. How > about leaving the code inside the loop just as we have it, and putting > if (StandbyMode) disable_startup_progress_timeout() before entering > the loop?
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. I'm attaching the v3 patch for further review. Please find the CF entry here - https://commitfest.postgresql.org/41/4012/. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
From 612f17d5e81181b69c3d711524de840c88cb3b4a Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> Date: Wed, 16 Nov 2022 06:20:15 +0000 Subject: [PATCH v3] 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 | 30 +++++++++++++++++++++-- src/backend/postmaster/startup.c | 29 +++++++++++++++++++--- src/include/postmaster/startup.h | 2 ++ 3 files changed, 56 insertions(+), 5 deletions(-) diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c index cb07694aea..0d57ea17ca 100644 --- a/src/backend/access/transam/xlogrecovery.c +++ b/src/backend/access/transam/xlogrecovery.c @@ -1644,9 +1644,24 @@ PerformWalRecovery(void) (errmsg("redo starts at %X/%X", LSN_FORMAT_ARGS(xlogreader->ReadRecPtr)))); - /* Prepare to report progress of the redo phase. */ - if (!StandbyMode) + if (StandbyMode) + { + /* + * 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(); + } + else + { + /* + * When not in standby mode, prepare to report progress of the redo + * phase. + */ begin_startup_progress_phase(); + } /* * main redo apply loop @@ -3115,8 +3130,19 @@ ReadRecord(XLogPrefetcher *xlogprefetcher, int emode, (errmsg_internal("reached end of WAL in pg_wal, entering archive recovery"))); InArchiveRecovery = true; if (StandbyModeRequested) + { 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(); + } + SwitchIntoArchiveRecovery(xlogreader->EndRecPtr, replayTLI); minRecoveryPoint = xlogreader->EndRecPtr; minRecoveryPointTLI = replayTLI; 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
