Subject: Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work

From: Andres Freund <[email-masked]>
Date: Tue, 15 Feb 2022 09:09:52 -0800

Hi,

On 2022-02-10 21:30:45 +0530, Bharath Rupireddy wrote:
> From 4801ff2c3b1e7bc7076205b676d4e3bc4a4ed308 Mon Sep 17 00:00:00 2001
> From: Bharath Rupireddy <[email-masked]>
> Date: Thu, 10 Feb 2022 15:58:58 +0000
> Subject: [PATCH v8] Replace ReadDir with ReadDirExtended
> 
> Replace ReadDir with ReadDirExtended (in CheckPointSnapBuild) and
> get rid of lstat entirely.

I think this might be based on a slight misunderstanding / bad phrasing on my
part.  We can use get_dirent_type() to optimize away the lstat on most
platforms, ReadDirExtended itself doesn't do that automatically. I was trying
to reference removing lstat calls by using get_dirent_type() in more places...

> We still use ReadDir in CheckPointLogicalRewriteHeap
> because unable to read directory would result a NULL from
> ReadDirExtended and we may miss to fsync the remaining map files,
> so here let's error out with ReadDir.

Then why is this skipping the lstat?

> Also, convert "could not parse filename" and "could not remove file"
> errors to LOG messages in  CheckPointLogicalRewriteHeap. This will
> enable checkpoint not to waste the amount of work that it had done.

I still doubt this is a good idea.

Greetings,

Andres Freund

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 15 Feb 2022 09:57:53 -0800

> I still doubt this is a good idea.

IIUC you are advocating for something more like the attached patches.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 8dddbf0241258e7b52ab664e077193d5008ee6cd Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Tue, 15 Feb 2022 09:40:53 -0800
Subject: [PATCH v9 1/3] make use of get_dirent_type() in replication code

---
 src/backend/access/heap/rewriteheap.c       | 4 ++--
 src/backend/replication/logical/snapbuild.c | 5 ++---
 src/backend/replication/slot.c              | 4 ++--
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 2a53826736..d64d7aae2e 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;
@@ -1227,7 +1227,7 @@ CheckPointLogicalRewriteHeap(void)
 			continue;
 
 		snprintf(path, sizeof(path), "pg_logical/mappings/%s", mapping_de->d_name);
-		if (lstat(path, &statbuf) == 0 && !S_ISREG(statbuf.st_mode))
+		if (get_dirent_type(path, mapping_de, false, ERROR) != PGFILETYPE_REG)
 			continue;
 
 		/* Skip over files that cannot be ours. */
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index 83fca8a77d..df50abfd98 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"
@@ -1947,15 +1948,13 @@ CheckPointSnapBuild(void)
 		uint32		hi;
 		uint32		lo;
 		XLogRecPtr	lsn;
-		struct stat statbuf;
 
 		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);
-
-		if (lstat(path, &statbuf) == 0 && !S_ISREG(statbuf.st_mode))
+		if (get_dirent_type(path, snap_de, false, ERROR) != 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 5da5fa825a..480a26bc7f 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"
@@ -1428,7 +1429,6 @@ StartupReplicationSlots(void)
 	replication_dir = AllocateDir("pg_replslot");
 	while ((replication_de = ReadDir(replication_dir, "pg_replslot")) != NULL)
 	{
-		struct stat statbuf;
 		char		path[MAXPGPATH + 12];
 
 		if (strcmp(replication_de->d_name, ".") == 0 ||
@@ -1438,7 +1438,7 @@ StartupReplicationSlots(void)
 		snprintf(path, sizeof(path), "pg_replslot/%s", replication_de->d_name);
 
 		/* we're only creating directories here, skip if it's not our's */
-		if (lstat(path, &statbuf) == 0 && !S_ISDIR(statbuf.st_mode))
+		if (get_dirent_type(path, replication_de, false, ERROR) != PGFILETYPE_DIR)
 			continue;
 
 		/* we crashed while a slot was being setup or deleted, clean up */
-- 
2.25.1

>From 1cb3fdc3e7ed323691ec37b76b95055ad9af9757 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Tue, 15 Feb 2022 09:46:44 -0800
Subject: [PATCH v9 2/3] add error checking for call to lstat() in replication
 code

---
 src/backend/replication/logical/reorderbuffer.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index c2d9be81fa..9b9e15858e 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -4812,7 +4812,11 @@ ReorderBufferCleanupSerializedTXNs(const char *slotname)
 	sprintf(path, "pg_replslot/%s", slotname);
 
 	/* we're only handling directories here, skip if it's not ours */
-	if (lstat(path, &statbuf) == 0 && !S_ISDIR(statbuf.st_mode))
+	if (lstat(path, &statbuf) != 0)
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("could not stat file \"%s\": %m", path)));
+	else if (!S_ISDIR(statbuf.st_mode))
 		return;
 
 	spill_dir = AllocateDir(path);
-- 
2.25.1

>From 7d2bed2e783aab2506bae27d1e4df90cc1b65dd4 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Tue, 15 Feb 2022 09:53:58 -0800
Subject: [PATCH v9 3/3] minor improvements to replication code

---
 src/backend/replication/logical/snapbuild.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index df50abfd98..6a60e942b7 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1965,16 +1965,10 @@ CheckPointSnapBuild(void)
 		 * everything but are postfixed by .$pid.tmp. We can just remove them
 		 * the same as other files because there can be none that are
 		 * currently being written that are older than cutoff.
-		 *
-		 * We just log a message if a file doesn't fit the pattern, it's
-		 * probably some editors lock/state file or similar...
 		 */
 		if (sscanf(snap_de->d_name, "%X-%X.snap", &hi, &lo) != 2)
-		{
-			ereport(LOG,
+			ereport(ERROR,
 					(errmsg("could not parse file name \"%s\"", path)));
-			continue;
-		}
 
 		lsn = ((uint64) hi) << 32 | lo;
 
@@ -1983,19 +1977,11 @@ CheckPointSnapBuild(void)
 		{
 			elog(DEBUG1, "removing snapbuild snapshot %s", path);
 
-			/*
-			 * It's not particularly harmful, though strange, if we can't
-			 * remove the file here. Don't prevent the checkpoint from
-			 * completing, that'd be a cure worse than the disease.
-			 */
 			if (unlink(path) < 0)
-			{
-				ereport(LOG,
+				ereport(ERROR,
 						(errcode_for_file_access(),
 						 errmsg("could not remove file \"%s\": %m",
 								path)));
-				continue;
-			}
 		}
 	}
 	FreeDir(snap_dir);
-- 
2.25.1

---

From: Andres Freund <[email-masked]>
Date: Tue, 15 Feb 2022 10:10:34 -0800

Hi,

> IIUC you are advocating for something more like the attached patches.

At least for patch 1 yes. Don't have the cycles just now to look at the
others.

I generally think it'd be a good exercise to go through an use
get_dirent_type() in nearly all ReadDir() style loops - it's a nice efficiency
win in general, and IIRC a particularly big one on windows.

It'd probably be good to add a reference to get_dirent_type() to
ReadDir[Extended]()'s docs.

Greetings,

Andres Freund

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 15 Feb 2022 10:37:58 -0800

> ReadDir[Extended]()'s docs.

Agreed.  I might give this a try.

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 15 Feb 2022 15:11:23 -0800

> Agreed.  I might give this a try.

Alright, here is a new patch set where I've tried to replace as many
stat()/lstat() calls as possible with get_dirent_type().  0002 and 0003 are
the same as v9.  I noticed a few remaining stat()/lstat() calls that don't
appear to be doing proper error checking, but I haven't had a chance to try
fixing those yet.

>From 23db5cb9bf04519b9341b6195ca224e0f8bc03cb Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Tue, 15 Feb 2022 09:40:53 -0800
Subject: [PATCH v10 1/3] make more use of get_dirent_type()

---
 src/backend/access/heap/rewriteheap.c       |  4 +-
 src/backend/replication/logical/snapbuild.c |  5 +--
 src/backend/replication/slot.c              |  4 +-
 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 +---
 7 files changed, 41 insertions(+), 63 deletions(-)

 

 

 
 		/* 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 14b77f2861..4a4a79b40a 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -2783,6 +2783,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)
@@ -2798,6 +2802,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)
@@ -3234,17 +3242,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 c70543fa74..8f10252fa4 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"
@@ -1018,7 +1019,7 @@ ParseConfigDirectory(const char *includedir,
 
 	while ((de = ReadDir(d, directory)) != NULL)
 	{
-		struct stat st;
+		PGFileType	de_type;
 		char		filename[MAXPGPATH];
 
 		/*
@@ -1035,32 +1036,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,
@@ -1068,6 +1046,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.25.1

>From 70c3ab552e729aeb84b006d0fdfa0df0e524b277 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Tue, 15 Feb 2022 09:46:44 -0800
Subject: [PATCH v10 2/3] add error checking for call to lstat() in replication
 code

---

 

>From d71585d0baca6c08be499fa7b6d42517e952d488 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Tue, 15 Feb 2022 09:53:58 -0800
Subject: [PATCH v10 3/3] minor improvements to replication code

---

 
 
-			/*

---

From: Thomas Munro <[email-masked]>
Date: Thu, 24 Mar 2022 13:17:01 +1300

> appear to be doing proper error checking, but I haven't had a chance to try

0001:  These get_dirent_type() conversions are nice.  LGTM.

0002:

Why is this a good place to silently ignore non-directories?
StartupReorderBuffer() is already in charge of skipping random
detritus found in the directory, so would it be better to do "if
(get_dirent_type(...) != PGFILETYPE_DIR) continue" there, and then
drop the lstat() stanza from ReorderBufferCleanupSeralizedTXNs()
completely?  Then perhaps its ReadDirExtended() shoud be using ERROR
instead of INFO, so that missing/non-dir/b0rked directories raise an
error.  I don't understand why it's reporting readdir() errors at INFO
but unlink() errors at ERROR, and as far as I can see the other paths
that reach this code shouldn't be sending in paths to non-directories
here unless something is seriously busted and that's ERROR-worthy.

0003:  I haven't studied this cure vs disease thing... no opinion today.

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 29 Mar 2022 15:48:32 -0700

Thanks for taking a look!

On Thu, Mar 24, 2022 at 01:17:01PM +1300, Thomas Munro wrote:
> error.

My guess is that this was done because ReorderBufferCleanupSerializedTXNs()
is also called from ReorderBufferAllocate() and ReorderBufferFree().
However, it is odd that we just silently return if the slot path isn't a
directory in those cases.  I think we could use get_dirent_type() in
StartupReorderBuffer() as you suggested, and then we could let ReadDir()
ERROR for non-directories for the other callers of
ReorderBufferCleanupSerializedTXNs().  WDYT?

> I don't understand why it's reporting readdir() errors at INFO

I agree.  I'll switch it to ReadDir() in the next revision so that we ERROR
instead of INFO.

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 30 Mar 2022 09:21:30 -0700

> I agree.  I'll switch it to ReadDir() in the next revision so that we ERROR

Here is an updated patch set.

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 8 Apr 2022 13:18:57 -0700

> Here is an updated patch set.

rebased

>From a92ccf47c9c334eea5b8d07b8fcab7031181c37e Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Tue, 15 Feb 2022 09:40:53 -0800
Subject: [PATCH v12 1/2] make more use of get_dirent_type()

---
 src/backend/access/heap/rewriteheap.c         |  4 +-
 .../replication/logical/reorderbuffer.c       | 12 +++---
 src/backend/replication/logical/snapbuild.c   |  5 +--
 src/backend/replication/slot.c                |  4 +-
 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, 48 insertions(+), 68 deletions(-)

 
 		/* Skip over files that cannot be ours. */
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 5adc016d44..63ef55f3f7 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -91,6 +91,7 @@
 #include "access/xact.h"
 #include "access/xlog_internal.h"
 #include "catalog/catalog.h"
+#include "common/file_utils.h"
 #include "lib/binaryheap.h"
 #include "miscadmin.h"
 #include "pgstat.h"
@@ -4408,15 +4409,10 @@ ReorderBufferCleanupSerializedTXNs(const char *slotname)
 {
 	DIR		   *spill_dir;
 	struct dirent *spill_de;
-	struct stat statbuf;
 	char		path[MAXPGPATH * 2 + 12];

-	/* we're only handling directories here, skip if it's not ours */
-	if (lstat(path, &statbuf) == 0 && !S_ISDIR(statbuf.st_mode))
-		return;
-
 	spill_dir = AllocateDir(path);
 	while ((spill_de = ReadDirExtended(spill_dir, path, INFO)) != NULL)
 	{
@@ -4464,6 +4460,7 @@ StartupReorderBuffer(void)
 {
 	DIR		   *logical_dir;
 	struct dirent *logical_de;
+	char		path[MAXPGPATH * 2 + 12];
 
 	logical_dir = AllocateDir("pg_replslot");
 	while ((logical_de = ReadDir(logical_dir, "pg_replslot")) != NULL)
@@ -4476,6 +4473,11 @@ StartupReorderBuffer(void)
 		if (!ReplicationSlotValidateName(logical_de->d_name, DEBUG2))
 			continue;
 
+		/* we're only handling directories here, skip if it's not ours */
+		sprintf(path, "pg_replslot/%s", logical_de->d_name);
+		if (get_dirent_type(path, logical_de, false, ERROR) != PGFILETYPE_DIR)
+			continue;
+
 		/*
 		 * ok, has to be a surviving logical slot, iterate and delete
 		 * everything starting with xid-*
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index 83fca8a77d..df50abfd98 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"
@@ -1947,15 +1948,13 @@ CheckPointSnapBuild(void)
 		uint32		hi;
 		uint32		lo;
 		XLogRecPtr	lsn;
-		struct stat statbuf;

index c35ea7c35b..5ee68e71b8 100644
 
@@ -1485,7 +1486,6 @@ StartupReplicationSlots(void)
 
 		if (strcmp(replication_de->d_name, ".") == 0 ||
@@ -1495,7 +1495,7 @@ StartupReplicationSlots(void)
 		snprintf(path, sizeof(path), "pg_replslot/%s", replication_de->d_name);

 

 

 

 
 		/*

 	if (num_filenames > 0)

 

>From 96b3c44bf9850b106751facd1e7d2d77543fe9d4 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Tue, 15 Feb 2022 09:53:58 -0800
Subject: [PATCH v12 2/2] minor improvements to replication code

---
 .../replication/logical/reorderbuffer.c        |  2 +-
 src/backend/replication/logical/snapbuild.c    | 18 ++----------------
 2 files changed, 3 insertions(+), 17 deletions(-)

diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 63ef55f3f7..532c3e4630 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -4414,7 +4414,7 @@ ReorderBufferCleanupSerializedTXNs(const char *slotname)
 	sprintf(path, "pg_replslot/%s", slotname);
 
 	spill_dir = AllocateDir(path);
-	while ((spill_de = ReadDirExtended(spill_dir, path, INFO)) != NULL)
+	while ((spill_de = ReadDir(spill_dir, path)) != NULL)
 	{
 		/* only look at names that can be ours */
 		if (strncmp(spill_de->d_name, "xid", 3) == 0)
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index df50abfd98..6a60e942b7 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1965,16 +1965,10 @@ CheckPointSnapBuild(void)
 		 * everything but are postfixed by .$pid.tmp. We can just remove them
 		 * the same as other files because there can be none that are
 		 * currently being written that are older than cutoff.
-		 *
-		 * We just log a message if a file doesn't fit the pattern, it's
-		 * probably some editors lock/state file or similar...
 		 */
 		if (sscanf(snap_de->d_name, "%X-%X.snap", &hi, &lo) != 2)
-		{
-			ereport(LOG,
+			ereport(ERROR,
 					(errmsg("could not parse file name \"%s\"", path)));
-			continue;
-		}

 
-			/*

---

From: Bharath Rupireddy <[email-masked]>
Date: Fri, 8 Jul 2022 21:39:10 +0530

> > Here is an updated patch set.

Thanks.

0001 - there are many places where lstat/stat is being used - don't we
need to replace all or most of them with get_dirent_type?

0002 - I'm not quite happy with this patch, with the change,
checkpoint errors out, if it can't remove just a file - the comments
there says it all. Is there any strong reason for this change?

- /*

Regards,
Bharath Rupireddy.

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 8 Jul 2022 10:14:39 -0700

> need to replace all or most of them with get_dirent_type?

It's been a while since I wrote this one, but I believe my intent was to
replace as many [l]stat() calls in ReadDir()-style loops as possible with
get_dirent_type().  Are there any that I've missed?

> there says it all. Is there any strong reason for this change?

Andres noted several concerns upthread.  In short, ignoring unexpected
errors makes them harder to debug and likely masks bugs.

FWIW I agree that it is unfortunate that a relatively non-critical error
here leads to checkpoint failures, which can cause much worse problems down
the road.  I think this is one of the reasons for moving tasks like this
out of the checkpointer, as I'm trying to do with the proposed custodian
process [0].

[0] https://commitfest.postgresql.org/38/3448/

---

From: Andres Freund <[email-masked]>
Date: Mon, 11 Jul 2022 13:25:33 -0700

Hi,

> +			filenames[num_filenames] = pstrdup(filename);

Seems like the diff would be easier to read if it didn't move code around as
much?

Looks pretty reasonable, I'd be happy to commit it, I think.

Greetings,

Andres Freund

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 11 Jul 2022 14:23:54 -0700

> Seems like the diff would be easier to read if it didn't move code around as

I opted to reorganize in order save an indent and simplify the conditions a
bit.  The alternative is something like this:

	if (de_type != PGFILETYPE_ERROR)
	{
		if (de_type != PGTFILETYPE_DIR)
		{
			...
		}
	}
	else
	{
		...
	}

I don't feel strongly one way or another, and I'll change it if you think
it's worth it to simplify the diff.

> Looks pretty reasonable, I'd be happy to commit it, I think.

Appreciate it.

---

From: Bharath Rupireddy <[email-masked]>
Date: Mon, 18 Jul 2022 16:53:18 +0530

> [0] https://commitfest.postgresql.org/38/3448/

IMHO, we can keep it as-is and if required can be changed as part of
the patch set [0], as this change without [0] can cause a checkpoint
to fail. Furthermore, I would like it if we convert "could not parse
filename" and "could not remove file" ERRORs of
CheckPointLogicalRewriteHeap to LOGs until [0] gets in - others may
have different opinions though.

Just wondering - do we ever have a problem if we can't remove the
snapshot or mapping file?

May be unrelated, RemoveTempXlogFiles doesn't even bother to check if
the temp wal file is removed.

Regards,
Bharath Rupireddy.

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 18 Jul 2022 11:18:42 -0700

> snapshot or mapping file?

Besides running out of disk space, there appears to be a transaction ID
wraparound risk with the mappings files.

---

From: Thomas Munro <[email-masked]>
Date: Tue, 9 Aug 2022 15:10:41 +1200

Working on get_dirent_type() reminded me of this thread.  I was going
to commit the first of these patches, but then I noticed Andres said
he was planning to, so I'll wait another day.  Here they are, with
commit messages but otherwise unchanged from Nathan's v12 except for a
slight comment tweak:

-               /* we're only handling directories here, skip if it's
not ours */
+               /* we're only handling directories here, skip if it's not one */

The only hunk I'm having second thoughts about is the following, which
makes unexpected stray files break checkpoints:

-                * We just log a message if a file doesn't fit the pattern, it's
-                * probably some editors lock/state file or similar...
                 */
                if (sscanf(snap_de->d_name, "%X-%X.snap", &hi, &lo) != 2)
-               {
-                       ereport(LOG,
+                       ereport(ERROR,
                                        (errmsg("could not parse file
name \"%s\"", path)));

Bharath mentioned other places that loop over stat(), but I think
those are places that want stuff we don't already have, like st_size.
From f0be5188830655b9ce9375b6d13b1ca728dbfcdd Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email-masked]>
Date: Tue, 9 Aug 2022 13:11:08 +1200
Subject: [PATCH v13 1/2] Make more use of get_dirent_type().

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.

In a couple of places this will now raise an ERROR if an internal
stat/lstat fails, where previously errors were silently ignored.  ENOENT
was not expected in these locations, because the loop in question is the
one responsible for unlinking files during checkpoint, or runs at
startup.

Author: Nathan Bossart <[email-masked]>
Reviewed-by: Thomas Munro <[email-masked]>
Reviewed-by: Bharath Rupireddy <[email-masked]>
Reviewed-by: Andres Freund <[email-masked]>
Discussion: https://postgr.es/m/20220215231123.GA2584239%40nathanxps13
---
 src/backend/access/heap/rewriteheap.c         |  4 +-
 .../replication/logical/reorderbuffer.c       | 12 +++---
 src/backend/replication/logical/snapbuild.c   |  5 +--
 src/backend/replication/slot.c                |  4 +-
 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, 48 insertions(+), 68 deletions(-)

index 9dd885d936..525039d5b2 100644

index 88a37fde72..f2b6cdf473 100644
@@ -4407,15 +4408,10 @@ ReorderBufferCleanupSerializedTXNs(const char *slotname)

@@ -4463,6 +4459,7 @@ StartupReorderBuffer(void)
 
 	logical_dir = AllocateDir("pg_replslot");
 	while ((logical_de = ReadDir(logical_dir, "pg_replslot")) != NULL)
@@ -4475,6 +4472,11 @@ StartupReorderBuffer(void)
 		if (!ReplicationSlotValidateName(logical_de->d_name, DEBUG2))
 			continue;
 
+		/* we're only handling directories here, skip if it's not one */
+		sprintf(path, "pg_replslot/%s", logical_de->d_name);
+		if (get_dirent_type(path, logical_de, false, ERROR) != PGFILETYPE_DIR)
+			continue;
+
 		/*
 		 * ok, has to be a surviving logical slot, iterate and delete
 		 * everything starting with xid-*
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index 73c0f15214..8587e2de43 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"
@@ -1946,15 +1947,13 @@ CheckPointSnapBuild(void)
 		uint32		hi;
 		uint32		lo;
 		XLogRecPtr	lsn;
-		struct stat statbuf;

index 850b74936f..bc51e816f9 100644
 
@@ -1442,7 +1443,6 @@ StartupReplicationSlots(void)
 
 		if (strcmp(replication_de->d_name, ".") == 0 ||
@@ -1452,7 +1452,7 @@ StartupReplicationSlots(void)
 		snprintf(path, sizeof(path), "pg_replslot/%s", replication_de->d_name);

 

 

index efb34d4dcb..64764a287d 100644
@@ -2708,6 +2708,10 @@ TryAgain:
@@ -2723,6 +2727,10 @@ ReadDir(DIR *dir, const char *dirname)
@@ -3159,17 +3167,11 @@ RemovePgTempFilesInDir(const char *tmpdirname, bool missing_ok, bool unlink_all)
 
index ce5633844c..88460422dd 100644
 
+#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,

 		/*
@@ -1034,32 +1035,9 @@ ParseConfigDirectory(const char *includedir,
 
@@ -1067,6 +1045,18 @@ ParseConfigDirectory(const char *includedir,
 
 	if (num_filenames > 0)

 

2.35.1

From 3e60b8362eaa5fc50093c3fc59bc16773dda550a Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email-masked]>
Date: Tue, 9 Aug 2022 14:42:10 +1200
Subject: [PATCH v13 2/2] Harden errors on some unexpected filesystem
 conditions.

CheckPointSnapBuild() shouldn't fail to unlink() files.  Raise an ERROR,
there's something wrong with the filesystem.  There is also no reason to
tolerate random stray files under pg_logical/snapshots.

ReorderBufferCleanupSerializedTXNs() shouldn't be called for a
non-directory, so raise an ERROR if opendir() fails, there's something
wrong with the filesystem.

Author: Nathan Bossart <[email-masked]>
Reviewed-by: Thomas Munro <[email-masked]>
Reviewed-by: Bharath Rupireddy <[email-masked]>
Reviewed-by: Andres Freund <[email-masked]>
Discussion: https://postgr.es/m/20220215231123.GA2584239%40nathanxps13
---
 .../replication/logical/reorderbuffer.c        |  2 +-
 src/backend/replication/logical/snapbuild.c    | 18 ++----------------
 2 files changed, 3 insertions(+), 17 deletions(-)

diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index f2b6cdf473..a80cbacd9b 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -4413,7 +4413,7 @@ ReorderBufferCleanupSerializedTXNs(const char *slotname)
 	sprintf(path, "pg_replslot/%s", slotname);
 
index 8587e2de43..bb788e1adc 100644
@@ -1964,16 +1964,10 @@ CheckPointSnapBuild(void)

@@ -1982,19 +1976,11 @@ CheckPointSnapBuild(void)
 		{
 			elog(DEBUG1, "removing snapbuild snapshot %s", path);
 
-			/*
2.35.1

---

From: Tom Lane <[email-masked]>
Date: Mon, 08 Aug 2022 23:17:30 -0400

> makes unexpected stray files break checkpoints:

Sounds like a pretty bad idea.  What's the upside?

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Mon, 08 Aug 2022 23:27:17 -0400

> Sounds like a pretty bad idea.  What's the upside?

Actually, having now read the patch, I don't think there is any
part of 0002 that is a good idea.  It's blithely removing the
comments that explain why the existing coding is the way it is,
and not providing a shred of justification for making checkpoints
more brittle.

I have not tried to analyze the error-handling properties of 0001,
but if it's being equally cavalier then it shouldn't be committed
either.  Most of this behavior is the result of decades of hard-won
experience; discarding it because it doesn't fit conveniently
into some refactoring plan isn't smart.

			regards, tom lane

---

From: Thomas Munro <[email-masked]>
Date: Tue, 9 Aug 2022 15:41:14 +1200

> and not providing a shred of justification for making checkpoints

0002 also contradicts the original $SUBJECT and goal of this thread,
which is possibly why it was kept separate.  I was only thinking of
committing 0001 myself, which is the one I'd reviewed an earlier
version of.

> into some refactoring plan isn't smart.

0001 does introduce new errors, as mentioned in the commit message, in
the form of elevel ERROR passed into get_dirent_type(), which might be
thrown if your OS has no d_type and lstat() fails (also if you asked
to follow symlinks, but in those cases errors were already thrown).
But in those cases, it seems at least a little fishy that we ignored
errors from the existing lstat().  I wondered if that was because they
expected that any failure meant ENOENT and they wanted to tolerate
that, but that does not seem to be the case, so I considered the error
to be an improvement.

---

From: Tom Lane <[email-masked]>
Date: Mon, 08 Aug 2022 23:50:18 -0400

Thomas Munro <[email-masked]> writes:
>> either.

> errors from the existing lstat().

Hmmm ... I'll grant that ignoring lstat errors altogether isn't great.
But should the replacement behavior be elog-LOG-and-press-on,
or elog-ERROR-and-fail-the-surrounding-operation?  I'm not in any
hurry to believe that the latter is more appropriate without some
analysis of what the callers are doing.

The bottom line here is that I'm distrustful of behavioral changes
introduced to simplify refactoring rather than to solve a live
problem.

			regards, tom lane

---

From: Bharath Rupireddy <[email-masked]>
Date: Tue, 9 Aug 2022 09:29:16 +0530

> introduced to simplify refactoring rather than to solve a live

+1. I agree with Tom not to change elog-LOG to elog-ERROR and fail the
checkpoint operation. Because the checkpoint is more important than
why a single snapshot file (out thousands or even million files) isn't
removed at that moment. Also, I originally proposed to change
elog-ERROR to elog-LOG in CheckPointLogicalRewriteHeap for unlink()
failures for the same reason.

--
Bharath Rupireddy
RDS Open Source Databases: https://aws.amazon.com/rds/postgresql/

---

From: Andres Freund <[email-masked]>
Date: Mon, 8 Aug 2022 21:07:54 -0700

On 2022-08-09 15:10:41 +1200, Thomas Munro wrote:
> I was going to commit the first of these patches, but then I noticed Andres
> said he was planning to, so I'll wait another day.

I'd be happy for you to take this on...

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 8 Aug 2022 21:28:02 -0700

> failures for the same reason.

This was my initial instinct as well, but this thread has received
contradictory feedback during the months since.

---

From: Thomas Munro <[email-masked]>
Date: Tue, 9 Aug 2022 17:26:39 +1200

> > checkpoint operation. Because the checkpoint is more important than

The changes were not from elog-LOG to elog-ERROR, they were changes
from silently eating errors, but I take your (plural) general point.

> contradictory feedback during the months since.

OK, so there aren't many places in 0001 that error out where we
previously did not.

For the one in CheckPointLogicalRewriteHeap(), if it fails, you don't
just want to skip -- it might be in the range that we need to fsync().
So what if we just deleted that get_dirent_type() != PGFILETYPE_REG
check altogether?  Depending on the name range check, we'd either
attempt to unlink() and LOG if that fails, or we'd attempt to
open()-or-ERROR and then fsync()-or-PANIC.  What functionality have we
lost without the DT_REG check?  Well, now if someone ill-advisedly
puts an important character device, socket, symlink (ie any kind of
non-DT_REG) into that directory with a name conforming to the
unlinkable range, we'll try to unlink it and possibly succeed, where
previously we'd have skipped, and if it's in the checkpointable range,
we'd try to fsync() it and likely fail, which is good because this is
a supposed to be a checkpoint.

That's already what happens if lstat() fails in master.  We fall back
to treating it like a DT_REG anyway:

               if (lstat(path, &statbuf) == 0 && !S_ISREG(statbuf.st_mode))
                        continue;

For the one in CheckSnapBuild(), it's similar but unlink only.

For the one in StartupReplicationSlots(), you could possibly take the
same line: if it's not a directory, we'll try an rmdir() it anyway and
then emit a WARNING if that fails.  Again, that's exactly what master
would do if that lstat() failed.

In other words, if we're going to LOG and press on, maybe we should
just press on anyway.  Would the error message be any less
informative?  Would the risk of unlinking a character device that you
accidentally stored in
/clusters/pg16-main/pgdata/pg_logical/mappings/map-1234-5678 be a
problem for anyone?

Separately from that topic, it would be nice to have a comment in each
place where we tolerate unlink() failure to explain why that is
harmless except for wasted disk.

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 9 Aug 2022 11:38:24 -0700

> from silently eating errors, but I take your (plural) general point.

I think this was in reference to 0002.  My comment was, at least.

> open()-or-ERROR and then fsync()-or-PANIC.

It looks like CheckPointLogicalRewriteHeap() presently ERRORs when unlink()
fails.  However, CheckPointSnapBuild() only LOGs when unlink() fails.
Since mappings files contain transaction IDs, persistent unlink() failures
might lead to wraparound, but I don't think failing checkpoints will help
improve matters.

Bharath's original patch changed CheckPointLogicalRewriteHeap() to LOG on
sscanf() and unlink() failures.  IIUC things would work the way you suggest
if that was applied.

> What functionality have we

I don't know that I agree that the last part is good.  Presumably we don't
want checkpointing to fail forever because there's a random non-regular
file that can't be fsync'd.  But it's not clear why such files would exist
in the first place.  Presumably this isn't the sort of thing that Postgres
should be expected to support.

> /clusters/pg16-main/pgdata/pg_logical/mappings/map-1234-5678 be a

Probably not.

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 10 Aug 2022 12:44:54 +0530

> OK, so there aren't many places in 0001 that error out where we

Well, that's not true I believe. The 0001 patch introduces
get_dirent_type() with elevel ERROR which means that lstat failures
are now reported as ERRORs with ereport(ERROR, as opposed to
continuing on the HEAD.

so on.

The main idea of using get_dirent_type() instead of lstat or stat is
to benefit from avoiding system calls on platforms where the directory
entry type is stored in dirent structure, but not to cause errors for
lstat or stat system calls failures where we previously didn't. I
think 0001 must be just replacing lstat or stat with get_dirent_type()
with elevel ERROR if lstat or stat failure is treated as ERROR
previously, otherwise with elevel LOG. I modified it as attached. Once
this patch gets committed, then we can continue the discussion as to
whether we make elog-LOG to elog-ERROR or vice-versa.

I'm tempted to use get_dirent_type() in RemoveXlogFile() but that
requires us to pass struct dirent * from RemoveOldXlogFiles() (as
attached 0002 for now), If done, this avoids one lstat() system call
per WAL file. IMO, that's okay to pass an extra function parameter to
RemoveXlogFile() as it is a static function and there can be many
(thousands of) WAL files at times, so saving system calls (lstat() *
number of WAL files) is definitely an advantage.

Thoughts?

---

From: Thomas Munro <[email-masked]>
Date: Wed, 10 Aug 2022 20:41:04 +1200

On Wed, Aug 10, 2022 at 7:15 PM Bharath Rupireddy
<[email-masked]> wrote:
> lstat or stat system calls failures where we previously didn't.

Yeah, I get it... I'm OK with separating mechanical changes like
lstat() -> get_dirent_type() from policy changes on error handling.  I
initially thought the ERROR was surely better than what we're doing
now (making extra system calls to avoid unlinking unexpected things,
for which our only strategy on failure is to try to unlink the thing
anyway), but it's better to discuss that separately.

> I

Agreed, will look at this version tomorrow.

> number of WAL files) is definitely an advantage.

-        lstat(path, &statbuf) == 0 && S_ISREG(statbuf.st_mode) &&
+        get_dirent_type(path, xlde, false, LOG) != PGFILETYPE_REG &&

I think you wanted ==, but maybe it'd be nicer to pass in a
"recyclable" boolean to RemoveXlogFile()?

If you're looking for more, rmtree() looks like another.

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 10 Aug 2022 15:28:25 +0530

Thanks.

> "recyclable" boolean to RemoveXlogFile()?

Ah, my bad, I corrected it now.

If you mean to do "bool recyclable = (get_dirent_type(path, xlde,
false, LOG) == PGFILETYPE_REG)"" and pass it as parameter to
RemoveXlogFile(), then we have to do get_dirent_type calls for every
WAL file even when any of (wal_recycle && *endlogSegNo <= recycleSegNo
&& XLogCtl->InstallXLogFileSegmentActive) is false which is
unnecessary and it's better to pass dirent structure as a parameter.

> If you're looking for more, rmtree() looks like another.

Are you suggesting to not use pgfnames() in rmtree() and do
opendir()-readdir()-get_dirent_type() directly? If yes, I don't think
it's a great idea because rmtree() has recursive calls and holding
opendir()-readdir() for all rmtree() recursive calls without
closedir() may eat up dynamic memory if there are deeper level
directories. I would like to leave it that way. Do you have any other
ideas in mind?

Please find the v15 patch, I merged the RemoveXlogFile() changes into 0001.

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 16 Aug 2022 13:32:33 -0700

On Wed, Aug 10, 2022 at 03:28:25PM +0530, Bharath Rupireddy wrote:
> +		if (get_dirent_type(path, mapping_de, false, LOG) != PGFILETYPE_REG)
>  			continue;

Previously, failure to lstat() wouldn't lead to skipping the entry.  With
this patch, a failure to determine the file type will cause the entry to be
skipped.  This might be okay in some places (e.g., CheckPointSnapBuild())
but not in others.  For example, in CheckPointLogicalRewriteHeap(), this
could cause us to skip fsync-ing a file due to a get_dirent_type() failure,
which seems bad.

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 17 Aug 2022 12:39:26 +0530

> could cause us to skip fsync-ing a file due to a get_dirent_type() failure,

Hm. I corrected it in the v16 patch, please review.

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 23 Aug 2022 11:07:44 -0700

On Wed, Aug 17, 2022 at 12:39:26PM +0530, Bharath Rupireddy wrote:
> +	/*
> +	 * We're only handling directories here, skip if it's not ours. Also, skip
> +	 * if the caller has already performed this check.
> +	 */
> +	if (!slot_dir_checked &&
> +		lstat(path, &statbuf) == 0 && !S_ISDIR(statbuf.st_mode))
>  		return;

There was previous discussion about removing this stanza from
ReorderBufferCleanupSeralizedTXNs() completely [0].  If that is still
possible, I think I would choose that over having callers specify whether
to do the directory check.

> +		if (get_dirent_type(path, logical_de, false, LOG) != PGFILETYPE_DIR)
> +			continue;

IIUC an error in get_dirent_type() could cause slots to be skipped here,
which is a behavior change.

[0] https://postgr.es/m/20220329224832.GA560657%40nathanxps13

---

From: Bharath Rupireddy <[email-masked]>
Date: Thu, 25 Aug 2022 10:48:08 +0530

---

From: Michael Paquier <[email-masked]>
Date: Thu, 25 Aug 2022 21:21:47 +0900

On Thu, Aug 25, 2022 at 10:48:08AM +0530, Bharath Rupireddy wrote:
> On Tue, Aug 23, 2022 at 11:37 PM Nathan Bossart <[email-masked]> wrote:
> That behaviour hasn't changed, no? Currently, if lstat() fails in
> ReorderBufferCleanupSerializedTXNs() it returns to
> StartupReorderBuffer()'s while loop which is in a way continuing with
> the other slots, this patch does nothing new.

Are you sure?  FWIW, the changes in reorderbuffer.c for
ReorderBufferCleanupSerializedTXNs() reduce the code readability, in
my opinion, so that's one less argument in favor of this change.

The gain in ParseConfigDirectory() is kind of cool.
pg_tzenumerate_next(), copydir(), RemoveXlogFile()
StartupReplicationSlots(), CheckPointLogicalRewriteHeap() and
RemovePgTempFilesInDir() seem fine, as well.  At least these avoid
extra lstat() calls when the file type is unknown, which would be only
a limited number of users where some of the three DT_* are missing
(right?).
--
Michael

---

From: Bharath Rupireddy <[email-masked]>
Date: Sat, 27 Aug 2022 14:06:32 +0530

On Thu, Aug 25, 2022 at 5:51 PM Michael Paquier <[email-masked]> wrote:
>
> FWIW, the changes in reorderbuffer.c for

Agreed. I reverted the changes.

> a limited number of users where some of the three DT_* are missing

Yes, the idea is to avoid lstat() system calls when possible.

PSA v17 patch with reorderbuffer.c changes reverted.

---

From: Nathan Bossart <[email-masked]>
Date: Sun, 28 Aug 2022 14:52:43 -0700

> PSA v17 patch with reorderbuffer.c changes reverted.

LGTM

---

From: Michael Paquier <[email-masked]>
Date: Wed, 31 Aug 2022 16:10:59 +0900

>> PSA v17 patch with reorderbuffer.c changes reverted.

Yeah, what you have here looks pretty fine to me, so this is IMO
committable.  Let's wait a bit to see if there are any objections from
the others, in case.
--
Michael

---

From: Andres Freund <[email-masked]>
Date: Wed, 31 Aug 2022 12:14:33 -0700

Hi,

>  			copy_file(fromfile, tofile);

It continues to make no sense to me to add behaviour changes around
error-handling as part of a conversion to get_dirent_type(). I don't at all
understand why e.g. the above change to make copydir() silently skip over
files it can't stat is ok?

Greetings,

Andres Freund

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 31 Aug 2022 12:24:28 -0700

> files it can't stat is ok?

In this example, the call to get_dirent_type() should ERROR if the call to
lstat() fails (the "elevel" argument is set to ERROR).
-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

---

From: Andres Freund <[email-masked]>
Date: Wed, 31 Aug 2022 12:28:18 -0700

Hi,

> lstat() fails (the "elevel" argument is set to ERROR).

Oh, oops. Skimmed code too quickly...

Greetings,

Andres Freund

---

From: Michael Paquier <[email-masked]>
Date: Fri, 2 Sep 2022 17:09:14 +0900

> committable.  Let's wait a bit to see if there are any objections from

I had a few hours and I've spent them looking at what you had here in
details, and there were a few things I have tweaked before applying
the patch.  First, elevel was set to LOG for three calls of
get_dirent_type() in code paths where we want to skip entries.  This
would have become very noisy, so I've switched two of them to DEBUG1
and the third one to DEBUG2 for consistency with the surroundings.  A
second thing was RemoveXlogFile(), where we passed down a dirent entry
*and* its d_name.  With this design, it would be easy to mess up
things and pass down a file name that does not match with its dirent
entry, so I have finished by replacing "segname" by the dirent
structure.  A last thing was about the two extra comment blocks in
fd.c, and I could not convince myself that these were helpful hints so
I have removed both of them.
--
Michael

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 2 Sep 2022 15:08:54 -0700

On Fri, Sep 02, 2022 at 05:09:14PM +0900, Michael Paquier wrote:
> the patch.

Thanks!

---

From: Ibrar Ahmed <[email-masked]>
Date: Sat, 3 Sep 2022 10:47:32 +0500

> > details, and there were a few things I have tweaked before applying

Your patch require a rebase. Please provide the latest rebase patch.

=== applying patch
./v17-0001-Make-more-use-of-get_dirent_type-in-place-of-sta.patch
patching file src/backend/access/heap/rewriteheap.c
Hunk #1 FAILED at 113.
Hunk #2 FAILED at 1213.
Hunk #3 FAILED at 1221.
3 out of 3 hunks FAILED -- saving rejects to file
src/backend/access/heap/rewriteheap.c.rej

-- 
Ibrar Ahmed

---

From: Michael Paquier <[email-masked]>
Date: Sat, 3 Sep 2022 17:06:19 +0900

> Your patch require a rebase. Please provide the latest rebase patch.

I was looking for a CF entry yesterday when I looked at this patch,
missing https://commitfest.postgresql.org/39/3496/.  This has been
applied as of bfb9dfd.
--
Michael