On Tue, Jan 20, 2015 at 1:56 AM, Andres Freund <and...@2ndquadrant.com> wrote:
> On 2015-01-19 17:16:11 +0900, Michael Paquier wrote:
>> On Mon, Jan 19, 2015 at 4:10 PM, Michael Paquier
>> <michael.paqu...@gmail.com> wrote:
>> > On Sat, Jan 17, 2015 at 2:44 AM, Andres Freund <and...@2ndquadrant.com>
>> > wrote:
>> >> Not this patch's fault, but I'm getting a bit tired seeing the above open
>> >> coded. How about adding a function that does the sleeping based on a
>> >> timestamptz and a ms interval?
>> > You mean in plugins, right? I don't recall seeing similar patterns in
>> > other code paths of backend. But I think that we can use something
>> > like that in timestamp.c then because we need to leverage that between
>> > two timestamps, the last failure and now():
>> > TimestampSleepDifference(start_time, stop_time, internal_ms);
>> > Perhaps you have something else in mind?
>> >
>> > Attached is an updated patch.
>
>> Actually I came with better than last patch by using a boolean flag as
>> return value of TimestampSleepDifference and use
>> TimestampDifferenceExceeds directly inside it.
>
>> Subject: [PATCH] Add wal_availability_check_interval to control WAL fetching
>> on failure
>
> I think that name isn't a very good. And its isn't very accurate
> either.
> How about wal_retrieve_retry_interval? Not very nice, but I think it's
> still better than the above.
Fine for me.
>> + <varlistentry id="wal-availability-check-interval"
>> xreflabel="wal_availability_check_interval">
>> + <term><varname>wal_availability_check_interval</varname>
>> (<type>integer</type>)
>> + <indexterm>
>> + <primary><varname>wal_availability_check_interval</> recovery
>> parameter</primary>
>> + </indexterm>
>> + </term>
>> + <listitem>
>> + <para>
>> + This parameter specifies the amount of time to wait when
>> + WAL is not available for a node in recovery. Default value is
>> + <literal>5s</>.
>> + </para>
>> + <para>
>> + A node in recovery will wait for this amount of time if
>> + <varname>restore_command</> returns nonzero exit status code when
>> + fetching new WAL segment files from archive or when a WAL receiver
>> + is not able to fetch a WAL record when using streaming replication.
>> + </para>
>> + </listitem>
>> + </varlistentry>
>> +
>> </variablelist>
>
> Walreceiver doesn't wait that amount, but rather how long the connection
> is intact. And restore_command may or may not retry.
I changed this paragraph as follows:
+ This parameter specifies the amount of time to wait when a failure
+ occurred when reading WAL from a source (be it via streaming
+ replication, local <filename>pg_xlog</> or WAL archive) for a node
+ in standby mode, or when WAL is expected from a source. Default
+ value is <literal>5s</>.
Pondering more about this patch, I am getting the feeling that it is
not that much a win to explain in details in the doc each failure
depending on the source from which WAL is taken. But it is essential
to mention that the wait is done only for a node using standby mode.
Btw, I noticed two extra things that I think should be done for consistency:
- We should use as well wal_retrieve_retry_interval when waiting for
WAL to arrive after checking for the failures:
/*
- * Wait for more WAL to
arrive. Time out after 5 seconds,
- * like when polling the
archive, to react to a trigger
- * file promptly.
+ * Wait for more WAL to
arrive. Time out after
+ * wal_retrieve_retry_interval
ms, like when polling the archive
+ * to react to a trigger file promptly.
*/
WaitLatch(&XLogCtl->recoveryWakeupLatch,
WL_LATCH_SET
| WL_TIMEOUT,
- 5000L);
+
wal_retrieve_retry_interval * 1L);
-
Updated patch attached.
--
Michael
From e07949030a676b033f4a563cfaac4687bcc37dca Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@otacoo.com>
Date: Mon, 19 Jan 2015 16:08:48 +0900
Subject: [PATCH] Add wal_retrieve_retry_interval to control WAL retrieval at
recovery
This parameter allows to control the amount of time process will wait
for WAL from a source when a failure occurred for a standby node, or
for the amount of time to wait when WAL is expected from a source.
Default value is 5s.
---
doc/src/sgml/recovery-config.sgml | 17 +++++++++
src/backend/access/transam/recovery.conf.sample | 9 +++++
src/backend/access/transam/xlog.c | 50 +++++++++++++++++--------
src/backend/utils/adt/timestamp.c | 24 ++++++++++++
src/include/utils/timestamp.h | 2 +
5 files changed, 87 insertions(+), 15 deletions(-)
diff --git a/doc/src/sgml/recovery-config.sgml b/doc/src/sgml/recovery-config.sgml
index 0c64ff2..becb2d3 100644
--- a/doc/src/sgml/recovery-config.sgml
+++ b/doc/src/sgml/recovery-config.sgml
@@ -145,6 +145,23 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows
</listitem>
</varlistentry>
+ <varlistentry id="wal-retrieve-retry-interval" xreflabel="wal_retrieve_retry_interval">
+ <term><varname>wal_retrieve_retry_interval</varname> (<type>integer</type>)
+ <indexterm>
+ <primary><varname>wal_retrieve_retry_interval</> recovery parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ This parameter specifies the amount of time to wait for WAL to become
+ available when a failure occurred when reading WAL from a source (be
+ it via streaming replication, local <filename>pg_xlog</> or WAL archive)
+ for a node in standby mode, or when WAL is expected from a source.
+ Default value is <literal>5s</>.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</sect1>
diff --git a/src/backend/access/transam/recovery.conf.sample b/src/backend/access/transam/recovery.conf.sample
index b777400..458308c 100644
--- a/src/backend/access/transam/recovery.conf.sample
+++ b/src/backend/access/transam/recovery.conf.sample
@@ -58,6 +58,15 @@
#
#recovery_end_command = ''
#
+#
+# wal_retrieve_retry_interval
+#
+# specifies an optional internal to wait for WAL to become available when
+# a failure occurred when reading WAL from a source for a node in standby
+# mode, or when WAL is expected from a source.
+#
+#wal_retrieve_retry_interval = '5s'
+#
#---------------------------------------------------------------------------
# RECOVERY TARGET PARAMETERS
#---------------------------------------------------------------------------
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 629a457..76bd5e2 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -235,6 +235,7 @@ static TimestampTz recoveryTargetTime;
static char *recoveryTargetName;
static int recovery_min_apply_delay = 0;
static TimestampTz recoveryDelayUntilTime;
+static int wal_retrieve_retry_interval = 5000;
/* options taken from recovery.conf for XLOG streaming */
static bool StandbyModeRequested = false;
@@ -4881,6 +4882,26 @@ readRecoveryCommandFile(void)
(errmsg_internal("trigger_file = '%s'",
TriggerFile)));
}
+ else if (strcmp(item->name, "wal_retrieve_retry_interval") == 0)
+ {
+ const char *hintmsg;
+
+ if (!parse_int(item->value, &wal_retrieve_retry_interval, GUC_UNIT_MS,
+ &hintmsg))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("parameter \"%s\" requires a temporal value",
+ "wal_retrieve_retry_interval"),
+ hintmsg ? errhint("%s", _(hintmsg)) : 0));
+ ereport(DEBUG2,
+ (errmsg_internal("wal_retrieve_retry_interval = '%s'", item->value)));
+
+ if (wal_retrieve_retry_interval <= 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"%s\" must have a value strictly positive",
+ "wal_retrieve_retry_interval")));
+ }
else if (strcmp(item->name, "recovery_min_apply_delay") == 0)
{
const char *hintmsg;
@@ -10340,8 +10361,8 @@ static bool
WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
bool fetching_ckpt, XLogRecPtr tliRecPtr)
{
- static pg_time_t last_fail_time = 0;
- pg_time_t now;
+ TimestampTz now = GetCurrentTimestamp();
+ TimestampTz last_fail_time = now;
/*-------
* Standby mode is implemented by a state machine:
@@ -10351,7 +10372,8 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
* 2. Check trigger file
* 3. Read from primary server via walreceiver (XLOG_FROM_STREAM)
* 4. Rescan timelines
- * 5. Sleep 5 seconds, and loop back to 1.
+ * 5. Sleep for the internal of time specified by
+ * wal_retrieve_retry_interval, and loop back to 1.
*
* Failure to read from the current source advances the state machine to
* the next state.
@@ -10490,15 +10512,13 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
* machine, so we've exhausted all the options for
* obtaining the requested WAL. We're going to loop back
* and retry from the archive, but if it hasn't been long
- * since last attempt, sleep 5 seconds to avoid
- * busy-waiting.
+ * since last attempt, sleep the amount of time specified
+ * by wal_retrieve_retry_interval to avoid busy-waiting.
*/
- now = (pg_time_t) time(NULL);
- if ((now - last_fail_time) < 5)
- {
- pg_usleep(1000000L * (5 - (now - last_fail_time)));
- now = (pg_time_t) time(NULL);
- }
+ now = GetCurrentTimestamp();
+ if (TimestampSleepDifference(last_fail_time, now,
+ wal_retrieve_retry_interval))
+ now = GetCurrentTimestamp();
last_fail_time = now;
currentSource = XLOG_FROM_ARCHIVE;
break;
@@ -10653,13 +10673,13 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
}
/*
- * Wait for more WAL to arrive. Time out after 5 seconds,
- * like when polling the archive, to react to a trigger
- * file promptly.
+ * Wait for more WAL to arrive. Time out after
+ * wal_retrieve_retry_interval ms, like when polling the archive
+ * to react to a trigger file promptly.
*/
WaitLatch(&XLogCtl->recoveryWakeupLatch,
WL_LATCH_SET | WL_TIMEOUT,
- 5000L);
+ wal_retrieve_retry_interval * 1L);
ResetLatch(&XLogCtl->recoveryWakeupLatch);
break;
}
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 67e0cf9..2b11c42 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -1674,6 +1674,30 @@ TimestampDifferenceExceeds(TimestampTz start_time,
}
/*
+ * TimestampSleepDifference -- sleep for the amout of interval time
+ * specified, reduced by the difference between two timestamps.
+ * Returns true if sleep is done, false otherwise.
+ *
+ * Both inputs must be ordinary finite timestamps (in current usage,
+ * they'll be results from GetCurrentTimestamp()).
+ */
+bool
+TimestampSleepDifference(TimestampTz start_time,
+ TimestampTz stop_time,
+ int interval_msec)
+{
+ long secs;
+ int microsecs;
+
+ if (TimestampDifferenceExceeds(start_time, stop_time, interval_msec))
+ return false;
+
+ TimestampDifference(start_time, stop_time, &secs, µsecs);
+ pg_usleep(interval_msec * 1000L - (1000000L * secs + 1L * microsecs));
+ return true;
+}
+
+/*
* Convert a time_t to TimestampTz.
*
* We do not use time_t internally in Postgres, but this is provided for use
diff --git a/src/include/utils/timestamp.h b/src/include/utils/timestamp.h
index 70118f5..e4a7673 100644
--- a/src/include/utils/timestamp.h
+++ b/src/include/utils/timestamp.h
@@ -217,6 +217,8 @@ extern void TimestampDifference(TimestampTz start_time, TimestampTz stop_time,
extern bool TimestampDifferenceExceeds(TimestampTz start_time,
TimestampTz stop_time,
int msec);
+extern bool TimestampSleepDifference(TimestampTz start_time,
+ TimestampTz stop_time, int interval_msec);
/*
* Prototypes for functions to deal with integer timestamps, when the native
--
2.2.2
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
