Re: Change pgarch_readyXlog() to return .history files first

[Michael Paquier]

On Thu, Dec 20, 2018 at 01:57:30PM +0200, David Steele wrote:
> Good point.  The new patch uses IsTLHistoryFileName().

OK, I have been reviewing the patch and the logic is correct, still I
could not resist reducing the number of inner if's for readability.  I
also did not like the high-jacking of rlde->d_name so instead let's use
an intermediate variable to store the basename of a scanned entry.  The
format of the if/elif with comments in-between was not really consistent
with the common practice as well.  pg_indent has also been applied.

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

OK, so am I doing.

Attached is an updated patch.  Does that look fine to you?  The base
logic is unchanged, and after a promotion history files get archived
before the last partial segment.
--
Michael

diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index e88d545d65..bbd6311b35 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -695,11 +695,12 @@ 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 +716,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);
@@ -722,32 +724,54 @@ pgarch_readyXlog(char *xlog)
 	while ((rlde = ReadDir(rldir, XLogArchiveStatusDir)) != NULL)
 	{
 		int			basenamelen = (int) strlen(rlde->d_name) - 6;
+		char		basename[MAX_XFN_CHARS + 1];
+		bool		ishistory;
 
-		if (basenamelen >= MIN_XFN_CHARS &&
-			basenamelen <= MAX_XFN_CHARS &&
-			strspn(rlde->d_name, VALID_XFN_CHARS) >= basenamelen &&
-			strcmp(rlde->d_name + basenamelen, ".ready") == 0)
+		/* Ignore entries with unexpected number of characters */
+		if (basenamelen < MIN_XFN_CHARS ||
+			basenamelen > MAX_XFN_CHARS)
+			continue;
+
+		/* Ignore entries with unexpected characters */
+		if (strspn(rlde->d_name, VALID_XFN_CHARS) < basenamelen)
+			continue;
+
+		/* Ignore anything not suffixed with .ready */
+		if (strcmp(rlde->d_name + basenamelen, ".ready") != 0)
+			continue;
+
+		/* Truncate off the .ready */
+		memcpy(basename, rlde->d_name, basenamelen);
+		basename[basenamelen] = '\0';
+
+		/* Is this a history file? */
+		ishistory = IsTLHistoryFileName(basename);
+
+		/*
+		 * Consume the file to archive.  History files have the highest
+		 * priority.  If this is the first file or the first history file
+		 * ever, copy it.  In the presence of a history file already chosen as
+		 * target, ignore all other files except history files which have been
+		 * generated for an older timeline than what is already chosen as
+		 * target to archive.
+		 */
+		if (!found || (ishistory && !historyFound))
 		{
-			if (!found)
-			{
-				strcpy(newxlog, rlde->d_name);
-				found = true;
-			}
-			else
-			{
-				if (strcmp(rlde->d_name, newxlog) < 0)
-					strcpy(newxlog, rlde->d_name);
-			}
+			strcpy(newxlog, basename);
+			found = true;
+			historyFound = ishistory;
+		}
+		else if (ishistory || (!ishistory && !historyFound))
+		{
+			if (strcmp(basename, newxlog) < 0)
+				strcpy(newxlog, basename);
 		}
 	}
 	FreeDir(rldir);
 
 	if (found)
-	{
-		/* truncate off the .ready */
-		newxlog[strlen(newxlog) - 6] = '\0';
 		strcpy(xlog, newxlog);
-	}
+
 	return found;
 }
 

signature.asc
Description: PGP signature