On Fri, Feb 3, 2023 at 2:29 AM Robert Haas <robertmh...@gmail.com> wrote:
>
Thanks for looking at this.
> On Tue, Nov 22, 2022 at 6:05 AM Bharath Rupireddy
> <bharath.rupireddyforpostg...@gmail.com> wrote:
> > If we place just the Assert(!StandbyMode); in
> > enable_startup_progress_timeout(), it fails for
> > begin_startup_progress_phase() in ResetUnloggedRelations() because the
> > InitWalRecovery(), that sets StandbyMode to true, is called before
> > ResetUnloggedRelations() . However, with the if (StandbyMode) {
> > return; }, we fail to report progress of ResetUnloggedRelations() in a
> > standby, which isn't a good idea at all because we only want to
> > disable the timeout during the recovery's main loop.
>
> Ugh. Well, in that case, I guess my vote is to forget about this whole
> Assert business and just commit what you had in v4. Does that work for
> you?
Yes, it seems reasonable to me.
> Protecting against specifically the situation where we're in the
> standby's main redo apply loop is not really what I had in mind here,
> but this is already sort of weirdly complicated-looking, and making it
> more weirdly complicated-looking to achieve the kind of protection
> that I had in mind doesn't really seem worth it.
IMHO, the responsibility of whether or not to report progress of any
operation can lie with the developers writing features using the
progress reporting mechanism. The progress reporting mechanism can
just be independent of all that.
I took the v4 patch, added some comments and attached it as the v7
patch here. Please find it.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From 0ff4bc13db8abf70f8da158d11eecdfbe7a7e199 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Fri, 3 Feb 2023 03:30:49 +0000
Subject: [PATCH v7] 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
Author: Bharath Rupireddy
Reviewed-by: Robert Haas, Simon Riggs
Reviewed-by: Thomas Munro
Backpatch-through: 15
Discussion: https://www.postgresql.org/message-id/CA%2BhUKGKCHSffAj8zZJKJvNX7ygnQFxVD6wm1d-2j3fVw%2BMafPQ%40mail.gmail.com
---
src/backend/access/transam/xlogrecovery.c | 25 ++++++++++++++++---
src/backend/postmaster/startup.c | 30 ++++++++++++++++++++---
src/include/postmaster/startup.h | 2 ++
3 files changed, 51 insertions(+), 6 deletions(-)
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index 2a5352f879..dbe9394762 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -385,6 +385,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,
@@ -469,6 +470,24 @@ XLogRecoveryShmemInit(void)
ConditionVariableInit(&XLogRecoveryCtl->recoveryNotPausedCV);
}
+/*
+ * A thin wrapper to enable StandbyMode and do other preparatory work as
+ * needed.
+ */
+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.
*
@@ -602,7 +621,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
@@ -739,7 +758,7 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
{
InArchiveRecovery = true;
if (StandbyModeRequested)
- StandbyMode = true;
+ EnableStandbyMode();
}
/* Get the last valid checkpoint record. */
@@ -3117,7 +3136,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 bcd23542f1..efc2580536 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,21 @@ 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 dd957f9291..6a2e4c4526 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
