Subject: Re: Change pgarch_readyXlog() to return .history files first
To: Michael Paquier <michael@paquier.xyz>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
References: <929068cf-69e1-bba2-9dc0-e05986aed471@pgmasters.net>
 <20181214001512.GB2921@paquier.xyz>
 <78169ccd-afaf-4247-9d69-01c32b7afabf@pgmasters.net>
 <20181215001033.GD5012@paquier.xyz>
From: David Steele <david@pgmasters.net>
Message-ID: <35420aa0-9e8b-66a2-3d69-d02cd4f8f6cf@pgmasters.net>
Date: Thu, 20 Dec 2018 13:57:30 +0200
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:60.0)
 Gecko/20100101 Thunderbird/60.3.3
MIME-Version: 1.0
In-Reply-To: <20181215001033.GD5012@paquier.xyz>
Content-Type: multipart/mixed;
 boundary="------------2586233B5B6BC6DE253C0718"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------2586233B5B6BC6DE253C0718
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit

On 12/15/18 2:10 AM, Michael Paquier wrote:
> On Fri, Dec 14, 2018 at 08:43:20AM -0500, David Steele wrote:
>> On 12/13/18 7:15 PM, Michael Paquier wrote:

>>> Or you could just use IsTLHistoryFileName here?
>>
>> We'd have to truncate .ready off the string to make that work, which
>> seems easy enough.  Is that what you were thinking?
> 
> Yes, that's the idea.  pgarch_readyXlog returns the segment name which
> should be archived, so you could just compute it after detecting a
> .ready file.
> 
>> One thing to consider is the check above is more efficient than
>> IsTLHistoryFileName() and it potentially gets run a lot.
> 
> This check misses strspn(), so any file finishing with .history would
> get eaten even if that's unlikely to happen.

Good point.  The new patch uses IsTLHistoryFileName().

>>> If one wants to simply check this code, you can just create dummy orphan
>>> files in archive_status and see in which order they get removed.
>>
>> Seems awfully racy.  Are there currently any tests like this for the
>> archiver that I can look at extending?
> 
> Sorry for the confusion, I was referring to manual testing here.

Ah, I see.  Yes, that's exactly how I tested it, in addition to doing 
real promotions.

> Thinking about it, we could have an automatic test to check for the file
> order pattern by creating dummy files, starting the server with archiver
> enabled, and then parse the logs as orphan .ready files would get
> removed in the order their archiving is attempted with one WARNING entry
> generated for each.  I am not sure if that is worth a test though.

Yes, parsing the logs was the best thing I could think of, too.

-- 
-David
david@pgmasters.net

--------------2586233B5B6BC6DE253C0718
Content-Type: text/plain; charset=UTF-8; x-mac-type="0"; x-mac-creator="0";
 name="history-files-first-v3.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="history-files-first-v3.patch"

From 1987a90b724506c7d9e453d9a976e3f982f625ec Mon Sep 17 00:00:00 2001
From: David Steele <david@pgmasters.net>
Date: Thu, 20 Dec 2018 13:28:51 +0200
Subject: [PATCH] Change pgarch_readyXlog() to return .history files first.

The alphabetical ordering of pgarch_readyXlog() means that on promotion
000000010000000100000001.partial will be archived before 00000002.history.

This appears harmless, but the .history files are what other potential primaries
use to decide what timeline they should pick.  The additional latency of
compressing/transferring the much larger partial file means that archiving of
the .history file is delayed and greatly increases the chance that another
primary will promote to the same timeline.

Teach pgarch_readyXlog() to return .history files first (and in order) to reduce
the window where this can happen.  This won't prevent all conflicts, but it is a
simple change and should greatly reduce real-world occurrences.
---
 src/backend/postmaster/pgarch.c | 36 +++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index e88d545d65..50dc2027b1 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -695,11 +695,11 @@ pgarch_archiveXlog(char *xlog)
  * 2) because the oldest ones will sooner become candidates for
  * recycling at time of checkpoint
  *
- * NOTE: the "oldest" comparison will presently consider all segments of
- * a timeline with a smaller ID to be older than all segments of a timeline
- * with a larger ID; the net result being that past timelines are given
- * higher priority for archiving.  This seems okay, or at least not
- * obviously worth changing.
+ * NOTE: the "oldest" comparison will consider any .history file to be older
+ * than any other file except another .history file.  Segments on a timeline
+ * with a smaller ID will be older than all segments on a timeline with a larger
+ * ID; the net result being that past timelines are given higher priority for
+ * archiving.  This seems okay, or at least not obviously worth changing.
  */
 static bool
 pgarch_readyXlog(char *xlog)
@@ -715,6 +715,7 @@ pgarch_readyXlog(char *xlog)
 	DIR		   *rldir;
 	struct dirent *rlde;
 	bool		found = false;
+	bool		historyFound = false;
 
 	snprintf(XLogArchiveStatusDir, MAXPGPATH, XLOGDIR "/archive_status");
 	rldir = AllocateDir(XLogArchiveStatusDir);
@@ -728,12 +729,28 @@ pgarch_readyXlog(char *xlog)
 			strspn(rlde->d_name, VALID_XFN_CHARS) >= basenamelen &&
 			strcmp(rlde->d_name + basenamelen, ".ready") == 0)
 		{
-			if (!found)
+            /* Truncate off the .ready */
+            rlde->d_name[basenamelen] = '\0';
+
+			/* Is this a history file? */
+			bool history = IsTLHistoryFileName(rlde->d_name);
+
+			/*
+			 * If this is the first file or the first history file, copy it
+			 */
+			if (!found || (history && !historyFound))
 			{
 				strcpy(newxlog, rlde->d_name);
 				found = true;
+				historyFound = history;
 			}
-			else
+			/*
+			 * Else compare and see if this file is alpabetically less than
+			 * what we already have.  If so, copy it.  History files always get
+			 * this comparison but other files only do when no history file has
+			 * been found yet.
+			 */
+			else if (history || (!history && !historyFound))
 			{
 				if (strcmp(rlde->d_name, newxlog) < 0)
 					strcpy(newxlog, rlde->d_name);
@@ -743,11 +760,8 @@ pgarch_readyXlog(char *xlog)
 	FreeDir(rldir);
 
 	if (found)
-	{
-		/* truncate off the .ready */
-		newxlog[strlen(newxlog) - 6] = '\0';
 		strcpy(xlog, newxlog);
-	}
+
 	return found;
 }
 
-- 
2.17.2 (Apple Git-113)


--------------2586233B5B6BC6DE253C0718--