v17-0001-Make-more-use-of-get_dirent_type-in-place-of-sta.patch

application/octet-stream

Filename: v17-0001-Make-more-use-of-get_dirent_type-in-place-of-sta.patch
Type: application/octet-stream
Part: 0
Message: Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work

Patch

Format: format-patch
Series: patch v17-0001
Subject: Make more use of get_dirent_type() in place of stat() or lstat()
File+
src/backend/access/heap/rewriteheap.c 5 2
src/backend/access/transam/xlog.c 9 8
src/backend/replication/logical/snapbuild.c 4 2
src/backend/replication/slot.c 4 2
src/backend/storage/file/copydir.c 6 15
src/backend/storage/file/fd.c 11 9
src/backend/utils/misc/guc-file.l 16 26
src/timezone/pgtz.c 2 6
From 3ce317e9eb4b37f79e3f822fd1ae53cb5d48b12e Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Sat, 27 Aug 2022 08:29:09 +0000
Subject: [PATCH v17] Make more use of get_dirent_type() in place of stat() or
 lstat()

We don't need to call stat() or lstat() in several ReadDir() loops, if
d_type is available, as it usually is on most systems.  If not,
get_dirent_type() will do that under the covers.

Authors: Nathan Bossart, Bharath Rupireddy
Reviewed-by: Thomas Munro, Andres Freund
Reviewed-by: Michael Paquier
Discussion: https://postgr.es/m/20220215231123.GA2584239%40nathanxps13
---
 src/backend/access/heap/rewriteheap.c       |  7 +++-
 src/backend/access/transam/xlog.c           | 17 +++++----
 src/backend/replication/logical/snapbuild.c |  6 ++-
 src/backend/replication/slot.c              |  6 ++-
 src/backend/storage/file/copydir.c          | 21 +++--------
 src/backend/storage/file/fd.c               | 20 +++++-----
 src/backend/utils/misc/guc-file.l           | 42 ++++++++-------------
 src/timezone/pgtz.c                         |  8 +---
 8 files changed, 57 insertions(+), 70 deletions(-)

diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 9dd885d936..5b20d1e1d5 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -113,6 +113,7 @@
 #include "access/xact.h"
 #include "access/xloginsert.h"
 #include "catalog/catalog.h"
+#include "common/file_utils.h"
 #include "lib/ilist.h"
 #include "miscadmin.h"
 #include "pgstat.h"
@@ -1213,7 +1214,6 @@ CheckPointLogicalRewriteHeap(void)
 	mappings_dir = AllocateDir("pg_logical/mappings");
 	while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
 	{
-		struct stat statbuf;
 		Oid			dboid;
 		Oid			relid;
 		XLogRecPtr	lsn;
@@ -1221,13 +1221,16 @@ CheckPointLogicalRewriteHeap(void)
 		TransactionId create_xid;
 		uint32		hi,
 					lo;
+		PGFileType	de_type;
 
 		if (strcmp(mapping_de->d_name, ".") == 0 ||
 			strcmp(mapping_de->d_name, "..") == 0)
 			continue;
 
 		snprintf(path, sizeof(path), "pg_logical/mappings/%s", mapping_de->d_name);
-		if (lstat(path, &statbuf) == 0 && !S_ISREG(statbuf.st_mode))
+		de_type = get_dirent_type(path, mapping_de, false, LOG);
+
+		if (de_type != PGFILETYPE_ERROR && de_type != PGFILETYPE_REG)
 			continue;
 
 		/* Skip over files that cannot be ours. */
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 537845cada..901169946d 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -657,8 +657,9 @@ static void PreallocXlogFiles(XLogRecPtr endptr, TimeLineID tli);
 static void RemoveTempXlogFiles(void);
 static void RemoveOldXlogFiles(XLogSegNo segno, XLogRecPtr lastredoptr,
 							   XLogRecPtr endptr, TimeLineID insertTLI);
-static void RemoveXlogFile(const char *segname, XLogSegNo recycleSegNo,
-						   XLogSegNo *endlogSegNo, TimeLineID insertTLI);
+static void RemoveXlogFile(const char *segname, const struct dirent *xlde,
+						   XLogSegNo recycleSegNo, XLogSegNo *endlogSegNo,
+						   TimeLineID insertTLI);
 static void UpdateLastRemovedPtr(char *filename);
 static void ValidateXLOGDirectoryStructure(void);
 static void CleanupBackupHistory(void);
@@ -3596,7 +3597,7 @@ RemoveOldXlogFiles(XLogSegNo segno, XLogRecPtr lastredoptr, XLogRecPtr endptr,
 				/* Update the last removed location in shared memory first */
 				UpdateLastRemovedPtr(xlde->d_name);
 
-				RemoveXlogFile(xlde->d_name, recycleSegNo, &endlogSegNo,
+				RemoveXlogFile(xlde->d_name, xlde, recycleSegNo, &endlogSegNo,
 							   insertTLI);
 			}
 		}
@@ -3669,7 +3670,7 @@ RemoveNonParentXlogFiles(XLogRecPtr switchpoint, TimeLineID newTLI)
 			 * - but seems safer to let them be archived and removed later.
 			 */
 			if (!XLogArchiveIsReady(xlde->d_name))
-				RemoveXlogFile(xlde->d_name, recycleSegNo, &endLogSegNo,
+				RemoveXlogFile(xlde->d_name, xlde, recycleSegNo, &endLogSegNo,
 							   newTLI);
 		}
 	}
@@ -3691,14 +3692,14 @@ RemoveNonParentXlogFiles(XLogRecPtr switchpoint, TimeLineID newTLI)
  * should be used for this timeline.
  */
 static void
-RemoveXlogFile(const char *segname, XLogSegNo recycleSegNo,
-			   XLogSegNo *endlogSegNo, TimeLineID insertTLI)
+RemoveXlogFile(const char *segname, const struct dirent *xlde,
+			   XLogSegNo recycleSegNo, XLogSegNo *endlogSegNo,
+			   TimeLineID insertTLI)
 {
 	char		path[MAXPGPATH];
 #ifdef WIN32
 	char		newpath[MAXPGPATH];
 #endif
-	struct stat statbuf;
 
 	snprintf(path, MAXPGPATH, XLOGDIR "/%s", segname);
 
@@ -3710,7 +3711,7 @@ RemoveXlogFile(const char *segname, XLogSegNo recycleSegNo,
 	if (wal_recycle &&
 		*endlogSegNo <= recycleSegNo &&
 		XLogCtl->InstallXLogFileSegmentActive &&	/* callee rechecks this */
-		lstat(path, &statbuf) == 0 && S_ISREG(statbuf.st_mode) &&
+		get_dirent_type(path, xlde, false, LOG) == PGFILETYPE_REG &&
 		InstallXLogFileSegment(endlogSegNo, path,
 							   true, recycleSegNo, insertTLI))
 	{
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index 1ff2c12240..f90059bb4e 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -123,6 +123,7 @@
 #include "access/heapam_xlog.h"
 #include "access/transam.h"
 #include "access/xact.h"
+#include "common/file_utils.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "replication/logical.h"
@@ -2049,15 +2050,16 @@ CheckPointSnapBuild(void)
 		uint32		hi;
 		uint32		lo;
 		XLogRecPtr	lsn;
-		struct stat statbuf;
+		PGFileType	de_type;
 
 		if (strcmp(snap_de->d_name, ".") == 0 ||
 			strcmp(snap_de->d_name, "..") == 0)
 			continue;
 
 		snprintf(path, sizeof(path), "pg_logical/snapshots/%s", snap_de->d_name);
+		de_type = get_dirent_type(path, snap_de, false, LOG);
 
-		if (lstat(path, &statbuf) == 0 && !S_ISREG(statbuf.st_mode))
+		if (de_type != PGFILETYPE_ERROR && de_type != PGFILETYPE_REG)
 		{
 			elog(DEBUG1, "only regular files expected: %s", path);
 			continue;
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 850b74936f..e9b926d7de 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -41,6 +41,7 @@
 
 #include "access/transam.h"
 #include "access/xlog_internal.h"
+#include "common/file_utils.h"
 #include "common/string.h"
 #include "miscadmin.h"
 #include "pgstat.h"
@@ -1442,17 +1443,18 @@ StartupReplicationSlots(void)
 	replication_dir = AllocateDir("pg_replslot");
 	while ((replication_de = ReadDir(replication_dir, "pg_replslot")) != NULL)
 	{
-		struct stat statbuf;
 		char		path[MAXPGPATH + 12];
+		PGFileType	de_type;
 
 		if (strcmp(replication_de->d_name, ".") == 0 ||
 			strcmp(replication_de->d_name, "..") == 0)
 			continue;
 
 		snprintf(path, sizeof(path), "pg_replslot/%s", replication_de->d_name);
+		de_type = get_dirent_type(path, replication_de, false, LOG);
 
 		/* we're only creating directories here, skip if it's not our's */
-		if (lstat(path, &statbuf) == 0 && !S_ISDIR(statbuf.st_mode))
+		if (de_type != PGFILETYPE_ERROR && de_type != PGFILETYPE_DIR)
 			continue;
 
 		/* we crashed while a slot was being setup or deleted, clean up */
diff --git a/src/backend/storage/file/copydir.c b/src/backend/storage/file/copydir.c
index 658fd95ba9..8a866191e1 100644
--- a/src/backend/storage/file/copydir.c
+++ b/src/backend/storage/file/copydir.c
@@ -22,6 +22,7 @@
 #include <unistd.h>
 #include <sys/stat.h>
 
+#include "common/file_utils.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "storage/copydir.h"
@@ -50,7 +51,7 @@ copydir(char *fromdir, char *todir, bool recurse)
 
 	while ((xlde = ReadDir(xldir, fromdir)) != NULL)
 	{
-		struct stat fst;
+		PGFileType	xlde_type;
 
 		/* If we got a cancel signal during the copy of the directory, quit */
 		CHECK_FOR_INTERRUPTS();
@@ -62,18 +63,15 @@ copydir(char *fromdir, char *todir, bool recurse)
 		snprintf(fromfile, sizeof(fromfile), "%s/%s", fromdir, xlde->d_name);
 		snprintf(tofile, sizeof(tofile), "%s/%s", todir, xlde->d_name);
 
-		if (lstat(fromfile, &fst) < 0)
-			ereport(ERROR,
-					(errcode_for_file_access(),
-					 errmsg("could not stat file \"%s\": %m", fromfile)));
+		xlde_type = get_dirent_type(fromfile, xlde, false, ERROR);
 
-		if (S_ISDIR(fst.st_mode))
+		if (xlde_type == PGFILETYPE_DIR)
 		{
 			/* recurse to handle subdirectories */
 			if (recurse)
 				copydir(fromfile, tofile, true);
 		}
-		else if (S_ISREG(fst.st_mode))
+		else if (xlde_type == PGFILETYPE_REG)
 			copy_file(fromfile, tofile);
 	}
 	FreeDir(xldir);
@@ -89,8 +87,6 @@ copydir(char *fromdir, char *todir, bool recurse)
 
 	while ((xlde = ReadDir(xldir, todir)) != NULL)
 	{
-		struct stat fst;
-
 		if (strcmp(xlde->d_name, ".") == 0 ||
 			strcmp(xlde->d_name, "..") == 0)
 			continue;
@@ -101,12 +97,7 @@ copydir(char *fromdir, char *todir, bool recurse)
 		 * We don't need to sync subdirectories here since the recursive
 		 * copydir will do it before it returns
 		 */
-		if (lstat(tofile, &fst) < 0)
-			ereport(ERROR,
-					(errcode_for_file_access(),
-					 errmsg("could not stat file \"%s\": %m", tofile)));
-
-		if (S_ISREG(fst.st_mode))
+		if (get_dirent_type(tofile, xlde, false, ERROR) == PGFILETYPE_REG)
 			fsync_fname(tofile, false);
 	}
 	FreeDir(xldir);
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index e3b19ca1ed..bd6f1270f9 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -2706,6 +2706,10 @@ TryAgain:
  *
  * The pathname passed to AllocateDir must be passed to this routine too,
  * but it is only used for error reporting.
+ *
+ * If you need to discover the file type of an entry, consider using
+ * get_dirent_type() (instead of stat()/lstat()) to avoid extra system calls on
+ * many platforms.
  */
 struct dirent *
 ReadDir(DIR *dir, const char *dirname)
@@ -2721,6 +2725,10 @@ ReadDir(DIR *dir, const char *dirname)
  * If elevel < ERROR, returns NULL after any error.  With the normal coding
  * pattern, this will result in falling out of the loop immediately as
  * though the directory contained no (more) entries.
+ *
+ * If you need to discover the file type of an entry, consider using
+ * get_dirent_type() (instead of stat()/lstat()) to avoid extra system calls on
+ * many platforms.
  */
 struct dirent *
 ReadDirExtended(DIR *dir, const char *dirname, int elevel)
@@ -3157,17 +3165,11 @@ RemovePgTempFilesInDir(const char *tmpdirname, bool missing_ok, bool unlink_all)
 					PG_TEMP_FILE_PREFIX,
 					strlen(PG_TEMP_FILE_PREFIX)) == 0)
 		{
-			struct stat statbuf;
+			PGFileType	type = get_dirent_type(rm_path, temp_de, false, LOG);
 
-			if (lstat(rm_path, &statbuf) < 0)
-			{
-				ereport(LOG,
-						(errcode_for_file_access(),
-						 errmsg("could not stat file \"%s\": %m", rm_path)));
+			if (type == PGFILETYPE_ERROR)
 				continue;
-			}
-
-			if (S_ISDIR(statbuf.st_mode))
+			else if (type == PGFILETYPE_DIR)
 			{
 				/* recursively remove contents, then directory itself */
 				RemovePgTempFilesInDir(rm_path, false, true);
diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
index ce5633844c..88460422dd 100644
--- a/src/backend/utils/misc/guc-file.l
+++ b/src/backend/utils/misc/guc-file.l
@@ -14,6 +14,7 @@
 #include <ctype.h>
 #include <unistd.h>
 
+#include "common/file_utils.h"
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
 #include "storage/fd.h"
@@ -1017,7 +1018,7 @@ ParseConfigDirectory(const char *includedir,
 
 	while ((de = ReadDir(d, directory)) != NULL)
 	{
-		struct stat st;
+		PGFileType	de_type;
 		char		filename[MAXPGPATH];
 
 		/*
@@ -1034,32 +1035,9 @@ ParseConfigDirectory(const char *includedir,
 
 		join_path_components(filename, directory, de->d_name);
 		canonicalize_path(filename);
-		if (stat(filename, &st) == 0)
+		de_type = get_dirent_type(filename, de, true, elevel);
+		if (de_type == PGFILETYPE_ERROR)
 		{
-			if (!S_ISDIR(st.st_mode))
-			{
-				/* Add file to array, increasing its size in blocks of 32 */
-				if (num_filenames >= size_filenames)
-				{
-					size_filenames += 32;
-					filenames = (char **) repalloc(filenames,
-											size_filenames * sizeof(char *));
-				}
-				filenames[num_filenames] = pstrdup(filename);
-				num_filenames++;
-			}
-		}
-		else
-		{
-			/*
-			 * stat does not care about permissions, so the most likely reason
-			 * a file can't be accessed now is if it was removed between the
-			 * directory listing and now.
-			 */
-			ereport(elevel,
-					(errcode_for_file_access(),
-					 errmsg("could not stat file \"%s\": %m",
-							filename)));
 			record_config_file_error(psprintf("could not stat file \"%s\"",
 											  filename),
 									 calling_file, calling_lineno,
@@ -1067,6 +1045,18 @@ ParseConfigDirectory(const char *includedir,
 			status = false;
 			goto cleanup;
 		}
+		else if (de_type != PGFILETYPE_DIR)
+		{
+			/* Add file to array, increasing its size in blocks of 32 */
+			if (num_filenames >= size_filenames)
+			{
+				size_filenames += 32;
+				filenames = (char **) repalloc(filenames,
+											   size_filenames * sizeof(char *));
+			}
+			filenames[num_filenames] = pstrdup(filename);
+			num_filenames++;
+		}
 	}
 
 	if (num_filenames > 0)
diff --git a/src/timezone/pgtz.c b/src/timezone/pgtz.c
index 3c278dd0f3..72f9e3d5e6 100644
--- a/src/timezone/pgtz.c
+++ b/src/timezone/pgtz.c
@@ -17,6 +17,7 @@
 #include <sys/stat.h>
 #include <time.h>
 
+#include "common/file_utils.h"
 #include "datatype/timestamp.h"
 #include "miscadmin.h"
 #include "pgtz.h"
@@ -429,7 +430,6 @@ pg_tzenumerate_next(pg_tzenum *dir)
 	{
 		struct dirent *direntry;
 		char		fullname[MAXPGPATH * 2];
-		struct stat statbuf;
 
 		direntry = ReadDir(dir->dirdesc[dir->depth], dir->dirname[dir->depth]);
 
@@ -447,12 +447,8 @@ pg_tzenumerate_next(pg_tzenum *dir)
 
 		snprintf(fullname, sizeof(fullname), "%s/%s",
 				 dir->dirname[dir->depth], direntry->d_name);
-		if (stat(fullname, &statbuf) != 0)
-			ereport(ERROR,
-					(errcode_for_file_access(),
-					 errmsg("could not stat \"%s\": %m", fullname)));
 
-		if (S_ISDIR(statbuf.st_mode))
+		if (get_dirent_type(fullname, direntry, true, ERROR) == PGFILETYPE_DIR)
 		{
 			/* Step into the subdirectory */
 			if (dir->depth >= MAX_TZDIR_DEPTH - 1)
-- 
2.34.1