A micro-optimisation for walkdir()
Hello hackers,
You don't need to call stat() just to find out if a dirent is a file
or directory, most of the time. Please see attached.
Attachments:
0001-Skip-unnecessary-stat-calls-in-walkdir.patchtext/x-patch; charset=US-ASCII; name=0001-Skip-unnecessary-stat-calls-in-walkdir.patchDownload
From cc2f0fd4a078728a67d862e2deec0332fb8b3555 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Wed, 2 Sep 2020 16:15:09 +1200
Subject: [PATCH] Skip unnecessary stat() calls in walkdir().
Often the kernel has already told us whether we have a file or a
directory, so we can avoid a call to stat().
---
src/backend/storage/file/fd.c | 57 +++++++++++++++++++++++++++--------
1 file changed, 44 insertions(+), 13 deletions(-)
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index f376a97ed6..99562f1ec7 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -3340,8 +3340,9 @@ walkdir(const char *path,
while ((de = ReadDirExtended(dir, path, elevel)) != NULL)
{
char subpath[MAXPGPATH * 2];
- struct stat fst;
- int sret;
+ bool is_unknown = false;
+ bool is_reg = false;
+ bool is_dir = false;
CHECK_FOR_INTERRUPTS();
@@ -3351,22 +3352,52 @@ walkdir(const char *path,
snprintf(subpath, sizeof(subpath), "%s/%s", path, de->d_name);
- if (process_symlinks)
- sret = stat(subpath, &fst);
- else
- sret = lstat(subpath, &fst);
+ /*
+ * We want to know if it's a file or directory. Some systems can tell
+ * us that already without an extra system call, but that's a BSD/GNU
+ * extension not mandated by POSIX, and even when the interface is
+ * present, sometimes the type is unknown, depending on the filesystem
+ * in use or in some cases options used at filesystem creation time.
+ */
+#if defined(DT_UNKNOWN) && defined(DT_REG) && defined(DT_DIR) && defined(DT_LNK)
+ if (de->d_type == DT_UNKNOWN ||
+ (de->d_type == DT_LNK && process_symlinks))
+ is_unknown = true;
+ else if (de->d_type == DT_REG)
+ is_reg = true;
+ else if (de->d_type == DT_DIR)
+ is_dir = true;
+#else
+ is_unknown = true;
+#endif
- if (sret < 0)
+ if (is_unknown)
{
- ereport(elevel,
- (errcode_for_file_access(),
- errmsg("could not stat file \"%s\": %m", subpath)));
- continue;
+ struct stat fst;
+ int sret;
+
+
+ if (process_symlinks)
+ sret = stat(subpath, &fst);
+ else
+ sret = lstat(subpath, &fst);
+
+ if (sret < 0)
+ {
+ ereport(elevel,
+ (errcode_for_file_access(),
+ errmsg("could not stat file \"%s\": %m", subpath)));
+ continue;
+ }
+ if (S_ISREG(fst.st_mode))
+ is_reg = true;
+ else if (S_ISDIR(fst.st_mode))
+ is_dir = true;
}
- if (S_ISREG(fst.st_mode))
+ if (is_reg)
(*action) (subpath, false, elevel);
- else if (S_ISDIR(fst.st_mode))
+ else if (is_dir)
walkdir(subpath, action, false, elevel);
}
--
2.20.1
Thomas Munro <thomas.munro@gmail.com> writes:
You don't need to call stat() just to find out if a dirent is a file
or directory, most of the time. Please see attached.
Hm. If we do this, I can see wanting to apply the knowledge in more
places than walkdir(). Is it possible to extract out the nonstandard
bits into a reusable subroutine? I'm envisioning an API more or less
like
extern enum PGFileType identify_file_type(const char *path,
const struct dirent *de,
bool look_thru_symlinks);
regards, tom lane
On Wed, Sep 2, 2020 at 4:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thomas Munro <thomas.munro@gmail.com> writes:
You don't need to call stat() just to find out if a dirent is a file
or directory, most of the time. Please see attached.Hm. If we do this, I can see wanting to apply the knowledge in more
places than walkdir().
Win32 could also benefit from this micro-optimisation if we expanded the
dirent port to include d_type. Please find attached a patch that does so.
Regards,
Juan José Santamaría Flecha
Attachments:
0002-Add-d_type-to-WIN32-dirent-port.patchapplication/octet-stream; name=0002-Add-d_type-to-WIN32-dirent-port.patchDownload
diff --git a/src/include/port/win32_msvc/dirent.h b/src/include/port/win32_msvc/dirent.h
index 9fabdf3..1d85f95 100644
--- a/src/include/port/win32_msvc/dirent.h
+++ b/src/include/port/win32_msvc/dirent.h
@@ -10,6 +10,7 @@ struct dirent
{
long d_ino;
unsigned short d_reclen;
+ unsigned char d_type;
unsigned short d_namlen;
char d_name[MAX_PATH];
};
@@ -20,4 +21,26 @@ DIR *opendir(const char *);
struct dirent *readdir(DIR *);
int closedir(DIR *);
+/* File types for 'd_type'. */
+enum
+ {
+ DT_UNKNOWN = 0,
+# define DT_UNKNOWN DT_UNKNOWN
+ DT_FIFO = 1,
+# define DT_FIFO DT_FIFO
+ DT_CHR = 2,
+# define DT_CHR DT_CHR
+ DT_DIR = 4,
+# define DT_DIR DT_DIR
+ DT_BLK = 6,
+# define DT_BLK DT_BLK
+ DT_REG = 8,
+# define DT_REG DT_REG
+ DT_LNK = 10,
+# define DT_LNK DT_LNK
+ DT_SOCK = 12,
+# define DT_SOCK DT_SOCK
+ DT_WHT = 14
+# define DT_WHT DT_WHT
+ };
#endif
diff --git a/src/port/dirent.c b/src/port/dirent.c
index b264484..519dd82 100644
--- a/src/port/dirent.c
+++ b/src/port/dirent.c
@@ -69,6 +69,7 @@ opendir(const char *dirname)
d->handle = INVALID_HANDLE_VALUE;
d->ret.d_ino = 0; /* no inodes on win32 */
d->ret.d_reclen = 0; /* not used on win32 */
+ d->ret.d_type = DT_UNKNOWN;
return d;
}
@@ -77,6 +78,7 @@ struct dirent *
readdir(DIR *d)
{
WIN32_FIND_DATA fd;
+ DWORD attrib;
if (d->handle == INVALID_HANDLE_VALUE)
{
@@ -105,6 +107,19 @@ readdir(DIR *d)
}
strcpy(d->ret.d_name, fd.cFileName); /* Both strings are MAX_PATH long */
d->ret.d_namlen = strlen(d->ret.d_name);
+ /*
+ * The only identifed types are: directory, regular file or symbolic link.
+ * Errors are treated as a file type that could not be determined.
+ */
+ attrib = GetFileAttributes(d->ret.d_name);
+ if (attrib == INVALID_FILE_ATTRIBUTES)
+ d->ret.d_type = DT_UNKNOWN;
+ else if ((attrib & FILE_ATTRIBUTE_DIRECTORY) != 0)
+ d->ret.d_type = DT_DIR;
+ else if ((attrib & FILE_ATTRIBUTE_REPARSE_POINT) != 0)
+ d->ret.d_type = DT_LNK;
+ else
+ d->ret.d_type = DT_REG;
return &d->ret;
}
On Thu, Sep 3, 2020 at 3:52 AM Juan José Santamaría Flecha
<juanjo.santamaria@gmail.com> wrote:
On Wed, Sep 2, 2020 at 4:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thomas Munro <thomas.munro@gmail.com> writes:
You don't need to call stat() just to find out if a dirent is a file
or directory, most of the time. Please see attached.Hm. If we do this, I can see wanting to apply the knowledge in more
places than walkdir().
Good idea. Here's a new version that defines a new function
get_dirent_type() in src/common/file_utils_febe.c and uses it for both
frontend and backend walkdir().
Win32 could also benefit from this micro-optimisation if we expanded the dirent port to include d_type. Please find attached a patch that does so.
Is GetFileAttributes() actually faster than stat()? Oh, I see that
our pgwin32_safestat() makes extra system calls. Huh. Ok then.
Thanks!
Attachments:
v2-0001-Skip-unnecessary-stat-calls-in-walkdir.patchtext/x-patch; charset=UTF-8; name=v2-0001-Skip-unnecessary-stat-calls-in-walkdir.patchDownload
From 19a5c980370f89d1340cdec7d74659715d8b4a17 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Thu, 3 Sep 2020 13:58:17 +1200
Subject: [PATCH v2 1/4] Skip unnecessary stat() calls in walkdir().
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Some kernels can tell us the type of a "dirent", so we can avoid a call
to stat() or lstat() in many cases. In order to be able to apply this
change to both frontend and backend versions of walkdir(), define a new
function get_dirent_type() in a new translation unit file_utils_febe.c.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
Discussion: https://postgr.es/m/CA%2BhUKG%2BFzxupGGN4GpUdbzZN%2Btn6FQPHo8w0Q%2BAPH5Wz8RG%2Bww%40mail.gmail.com
---
src/backend/storage/file/fd.c | 26 +++++------
src/common/Makefile | 1 +
src/common/file_utils.c | 28 +++++-------
src/common/file_utils_febe.c | 81 +++++++++++++++++++++++++++++++++
src/include/common/file_utils.h | 21 ++++++++-
src/tools/msvc/Mkvcbuild.pm | 2 +-
6 files changed, 126 insertions(+), 33 deletions(-)
create mode 100644 src/common/file_utils_febe.c
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index f376a97ed6..b9acd38d23 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -89,6 +89,7 @@
#include "access/xlog.h"
#include "catalog/pg_tablespace.h"
#include "common/file_perm.h"
+#include "common/file_utils.h"
#include "miscadmin.h"
#include "pgstat.h"
#include "portability/mem.h"
@@ -3340,8 +3341,6 @@ walkdir(const char *path,
while ((de = ReadDirExtended(dir, path, elevel)) != NULL)
{
char subpath[MAXPGPATH * 2];
- struct stat fst;
- int sret;
CHECK_FOR_INTERRUPTS();
@@ -3351,23 +3350,22 @@ walkdir(const char *path,
snprintf(subpath, sizeof(subpath), "%s/%s", path, de->d_name);
- if (process_symlinks)
- sret = stat(subpath, &fst);
- else
- sret = lstat(subpath, &fst);
-
- if (sret < 0)
+ switch (get_dirent_type(subpath, de, process_symlinks))
{
+ case PGFILETYPE_REG:
+ (*action) (subpath, false, elevel);
+ break;
+ case PGFILETYPE_DIR:
+ walkdir(subpath, action, false, elevel);
+ break;
+ case PGFILETYPE_ERROR:
ereport(elevel,
(errcode_for_file_access(),
errmsg("could not stat file \"%s\": %m", subpath)));
- continue;
+ break;
+ default:
+ break;
}
-
- if (S_ISREG(fst.st_mode))
- (*action) (subpath, false, elevel);
- else if (S_ISDIR(fst.st_mode))
- walkdir(subpath, action, false, elevel);
}
FreeDir(dir); /* we ignore any error here */
diff --git a/src/common/Makefile b/src/common/Makefile
index 16619e4ba8..aac92aabe1 100644
--- a/src/common/Makefile
+++ b/src/common/Makefile
@@ -56,6 +56,7 @@ OBJS_COMMON = \
exec.o \
f2s.o \
file_perm.o \
+ file_utils_febe.o \
hashfn.o \
ip.o \
jsonapi.o \
diff --git a/src/common/file_utils.c b/src/common/file_utils.c
index a2faafdf13..5a427e246c 100644
--- a/src/common/file_utils.c
+++ b/src/common/file_utils.c
@@ -2,7 +2,7 @@
*
* File-processing utility routines.
*
- * Assorted utility functions to work on files.
+ * Assorted utility functions to work on files, frontend only.
*
*
* Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
@@ -27,7 +27,6 @@
#include "common/file_utils.h"
#include "common/logging.h"
-
/* Define PG_FLUSH_DATA_WORKS if we have an implementation for pg_flush_data */
#if defined(HAVE_SYNC_FILE_RANGE)
#define PG_FLUSH_DATA_WORKS 1
@@ -167,8 +166,6 @@ walkdir(const char *path,
while (errno = 0, (de = readdir(dir)) != NULL)
{
char subpath[MAXPGPATH * 2];
- struct stat fst;
- int sret;
if (strcmp(de->d_name, ".") == 0 ||
strcmp(de->d_name, "..") == 0)
@@ -176,21 +173,20 @@ walkdir(const char *path,
snprintf(subpath, sizeof(subpath), "%s/%s", path, de->d_name);
- if (process_symlinks)
- sret = stat(subpath, &fst);
- else
- sret = lstat(subpath, &fst);
-
- if (sret < 0)
+ switch (get_dirent_type(subpath, de, process_symlinks))
{
- pg_log_error("could not stat file \"%s\": %m", subpath);
- continue;
- }
-
- if (S_ISREG(fst.st_mode))
+ case PGFILETYPE_REG:
(*action) (subpath, false);
- else if (S_ISDIR(fst.st_mode))
+ break;
+ case PGFILETYPE_DIR:
walkdir(subpath, action, false);
+ break;
+ case PGFILETYPE_ERROR:
+ pg_log_error("could not stat file \"%s\": %m", subpath);
+ break;
+ default:
+ break;
+ }
}
if (errno)
diff --git a/src/common/file_utils_febe.c b/src/common/file_utils_febe.c
new file mode 100644
index 0000000000..f4c84b8a64
--- /dev/null
+++ b/src/common/file_utils_febe.c
@@ -0,0 +1,81 @@
+/*-------------------------------------------------------------------------
+ *
+ * File-processing utility routines.
+ *
+ * Assorted utility functions to work on files, frontend and backend.
+ *
+ *
+ * Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/common/file_utils_febe.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#ifdef FRONTEND
+#include "postgres_fe.h"
+#else
+#include "postgres.h"
+#endif
+
+#include <dirent.h>
+#include <sys/stat.h>
+
+#include "common/file_utils.h"
+
+/*
+ * Return the type of a directory entry.
+ */
+PGFileType
+get_dirent_type(const char *path,
+ const struct dirent *de,
+ bool look_through_symlinks)
+{
+ PGFileType result;
+
+ /*
+ * We want to know the type of a directory entry. Some systems tell us
+ * that directly in the dirent struct, but that's a BSD/GNU extension.
+ * Even when the interface is present, sometimes the type is unknown,
+ * depending on the filesystem in use or in some cases options used at
+ * filesystem creation time.
+ */
+#if defined(DT_UNKNOWN) && defined(DT_REG) && defined(DT_DIR) && defined(DT_LNK)
+ if (de->d_type == DT_REG)
+ result = PGFILETYPE_REG;
+ else if (de->d_type == DT_DIR)
+ result = PGFILETYPE_DIR;
+ else if (de->d_type == DT_LNK && !look_through_symlinks)
+ result = PGFILETYPE_LNK;
+ else
+ result = PGFILETYPE_UNKNOWN;
+#else
+ result = PGFILETYPE_UNKNOWN;
+#endif
+
+ if (result == PGFILETYPE_UNKNOWN)
+ {
+ struct stat fst;
+ int sret;
+
+
+ if (look_through_symlinks)
+ sret = stat(path, &fst);
+ else
+ sret = lstat(path, &fst);
+
+ if (sret < 0)
+ result = PGFILETYPE_ERROR;
+ else if (S_ISREG(fst.st_mode))
+ result = PGFILETYPE_REG;
+ else if (S_ISDIR(fst.st_mode))
+ result = PGFILETYPE_DIR;
+#ifdef S_ISLNK
+ else if (S_ISLNK(fst.st_mode))
+ result = PGFILETYPE_LNK;
+#endif
+ }
+
+ return result;
+}
diff --git a/src/include/common/file_utils.h b/src/include/common/file_utils.h
index a7add75efa..d723c483d2 100644
--- a/src/include/common/file_utils.h
+++ b/src/include/common/file_utils.h
@@ -1,6 +1,4 @@
/*-------------------------------------------------------------------------
- *
- * File-processing utility routines for frontend code
*
* Assorted utility functions to work on files.
*
@@ -15,10 +13,29 @@
#ifndef FILE_UTILS_H
#define FILE_UTILS_H
+#include <dirent.h>
+
+typedef enum PGFileType
+{
+ PGFILETYPE_ERROR,
+ PGFILETYPE_UNKNOWN,
+ PGFILETYPE_REG,
+ PGFILETYPE_DIR,
+ PGFILETYPE_LNK
+} PGFileType;
+
+/* Functions defined in file_utils_febe.c for both FE and BE code. */
+extern PGFileType get_dirent_type(const char *path,
+ const struct dirent *de,
+ bool look_through_symlinks);
+
+/* Functions defined in file_utils.c only for FE code. */
+#ifdef FRONTEND
extern int fsync_fname(const char *fname, bool isdir);
extern void fsync_pgdata(const char *pg_data, int serverVersion);
extern void fsync_dir_recurse(const char *dir);
extern int durable_rename(const char *oldfile, const char *newfile);
extern int fsync_parent_path(const char *fname);
+#endif
#endif /* FILE_UTILS_H */
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 20da7985c1..a64127a196 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -121,7 +121,7 @@ sub mkvcbuild
our @pgcommonallfiles = qw(
archive.c base64.c checksum_helper.c
config_info.c controldata_utils.c d2s.c encnames.c exec.c
- f2s.c file_perm.c hashfn.c ip.c jsonapi.c
+ f2s.c file_perm.c file_utils_febe.c hashfn.c ip.c jsonapi.c
keywords.c kwlookup.c link-canary.c md5.c
pg_lzcompress.c pgfnames.c psprintf.c relpath.c rmtree.c
saslprep.c scram-common.c string.c stringinfo.c unicode_norm.c username.c
--
2.20.1
v2-0002-Add-d_type-to-Win32-dirent-port.patchtext/x-patch; charset=UTF-8; name=v2-0002-Add-d_type-to-Win32-dirent-port.patchDownload
From 1c4ea1bc1a23a9db1b48c8b83cfc44c376c5c811 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Thu, 3 Sep 2020 13:09:52 +1200
Subject: [PATCH v2 2/4] Add d_type to Win32 dirent port.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The member d_type is a BSD/GNU extension to struct dirent, but Windows
has the information required to populate it so let's add it to our
emulation so that get_dirent_type() can use it.
Author: Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
Discussion: https://postgr.es/m/CA%2BhUKG%2BFzxupGGN4GpUdbzZN%2Btn6FQPHo8w0Q%2BAPH5Wz8RG%2Bww%40mail.gmail.com
---
src/include/port/win32_msvc/dirent.h | 23 +++++++++++++++++++++++
src/port/dirent.c | 15 +++++++++++++++
2 files changed, 38 insertions(+)
diff --git a/src/include/port/win32_msvc/dirent.h b/src/include/port/win32_msvc/dirent.h
index 9fabdf332b..6a54f964f5 100644
--- a/src/include/port/win32_msvc/dirent.h
+++ b/src/include/port/win32_msvc/dirent.h
@@ -10,6 +10,7 @@ struct dirent
{
long d_ino;
unsigned short d_reclen;
+ unsigned char d_type;
unsigned short d_namlen;
char d_name[MAX_PATH];
};
@@ -20,4 +21,26 @@ DIR *opendir(const char *);
struct dirent *readdir(DIR *);
int closedir(DIR *);
+/* File types for 'd_type'. */
+enum
+ {
+ DT_UNKNOWN = 0,
+# define DT_UNKNOWN DT_UNKNOWN
+ DT_FIFO = 1,
+# define DT_FIFO DT_FIFO
+ DT_CHR = 2,
+# define DT_CHR DT_CHR
+ DT_DIR = 4,
+# define DT_DIR DT_DIR
+ DT_BLK = 6,
+# define DT_BLK DT_BLK
+ DT_REG = 8,
+# define DT_REG DT_REG
+ DT_LNK = 10,
+# define DT_LNK DT_LNK
+ DT_SOCK = 12,
+# define DT_SOCK DT_SOCK
+ DT_WHT = 14
+# define DT_WHT DT_WHT
+ };
#endif
diff --git a/src/port/dirent.c b/src/port/dirent.c
index b264484fca..519dd82101 100644
--- a/src/port/dirent.c
+++ b/src/port/dirent.c
@@ -69,6 +69,7 @@ opendir(const char *dirname)
d->handle = INVALID_HANDLE_VALUE;
d->ret.d_ino = 0; /* no inodes on win32 */
d->ret.d_reclen = 0; /* not used on win32 */
+ d->ret.d_type = DT_UNKNOWN;
return d;
}
@@ -77,6 +78,7 @@ struct dirent *
readdir(DIR *d)
{
WIN32_FIND_DATA fd;
+ DWORD attrib;
if (d->handle == INVALID_HANDLE_VALUE)
{
@@ -105,6 +107,19 @@ readdir(DIR *d)
}
strcpy(d->ret.d_name, fd.cFileName); /* Both strings are MAX_PATH long */
d->ret.d_namlen = strlen(d->ret.d_name);
+ /*
+ * The only identifed types are: directory, regular file or symbolic link.
+ * Errors are treated as a file type that could not be determined.
+ */
+ attrib = GetFileAttributes(d->ret.d_name);
+ if (attrib == INVALID_FILE_ATTRIBUTES)
+ d->ret.d_type = DT_UNKNOWN;
+ else if ((attrib & FILE_ATTRIBUTE_DIRECTORY) != 0)
+ d->ret.d_type = DT_DIR;
+ else if ((attrib & FILE_ATTRIBUTE_REPARSE_POINT) != 0)
+ d->ret.d_type = DT_LNK;
+ else
+ d->ret.d_type = DT_REG;
return &d->ret;
}
--
2.20.1
Thomas Munro <thomas.munro@gmail.com> writes:
On Wed, Sep 2, 2020 at 4:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Hm. If we do this, I can see wanting to apply the knowledge in more
places than walkdir().
Good idea. Here's a new version that defines a new function
get_dirent_type() in src/common/file_utils_febe.c and uses it for both
frontend and backend walkdir().
Quick thoughts on this patch:
* The API spec for get_dirent_type() needs to say that errno is
meaningful when the return value is PGFILETYPE_ERROR. That's
something that would not be hard to break, so not documenting
the point at all doesn't seem great. More generally, I don't
especially like having the callers know that the errno is from
stat() rather than something else.
* I don't quite like the calling code you have that covers some
return values and then has a default: case without any comment.
It's not really obvious that the default: case is expected to be
hit in non-error situations, especially when there is a separate
switch path for errors. I can't find fault with the code as such,
but I think it'd be good to have a comment there. Maybe along
the lines of "Ignore file types other than regular files and
directories".
Both of these concerns would abate if we had get_dirent_type()
just throw an error itself when stat() fails, thereby removing the
PGFILETYPE_ERROR result code. I'm not 100% sold either way on
that, but it's something to think about. Is there ever going
to be a reason for the caller to ignore an error?
regards, tom lane
On Thu, Sep 3, 2020 at 5:36 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
[request for better comments]
Ack.
Both of these concerns would abate if we had get_dirent_type()
just throw an error itself when stat() fails, thereby removing the
PGFILETYPE_ERROR result code. I'm not 100% sold either way on
that, but it's something to think about. Is there ever going
to be a reason for the caller to ignore an error?
Hmm. Well I had it like that in an earlier version, but then I
couldn't figure out the right way to write code that would work in
both frontend and backend code, without writing two copies in two
translation units, or putting the whole thing in a header. What
approach do you prefer?
Thomas Munro <thomas.munro@gmail.com> writes:
On Thu, Sep 3, 2020 at 5:36 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Both of these concerns would abate if we had get_dirent_type()
just throw an error itself when stat() fails, thereby removing the
PGFILETYPE_ERROR result code. I'm not 100% sold either way on
that, but it's something to think about. Is there ever going
to be a reason for the caller to ignore an error?
Hmm. Well I had it like that in an earlier version, but then I
couldn't figure out the right way to write code that would work in
both frontend and backend code, without writing two copies in two
translation units, or putting the whole thing in a header. What
approach do you prefer?
Well, there are plenty of places in src/port/ where we do things like
#ifndef FRONTEND
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not get junction for \"%s\": %s",
path, msg)));
#else
fprintf(stderr, _("could not get junction for \"%s\": %s\n"),
path, msg);
#endif
I don't see a compelling reason why this function couldn't report
stat() failures similarly, especially if we're just going to have
the callers do exactly the same thing as that anyway.
regards, tom lane
On Fri, Sep 4, 2020 at 3:31 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thomas Munro <thomas.munro@gmail.com> writes:
Hmm. Well I had it like that in an earlier version, but then I
couldn't figure out the right way to write code that would work in
both frontend and backend code, without writing two copies in two
translation units, or putting the whole thing in a header. What
approach do you prefer?Well, there are plenty of places in src/port/ where we do things like
#ifndef FRONTEND
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not get junction for \"%s\": %s",
path, msg)));
#else
fprintf(stderr, _("could not get junction for \"%s\": %s\n"),
path, msg);
#endifI don't see a compelling reason why this function couldn't report
stat() failures similarly, especially if we're just going to have
the callers do exactly the same thing as that anyway.
Ok, so the main weird thing is that you finish up having to pass in an
elevel, but that has different meanings in FE and BE code. Note that
you still need a PGFILE_ERROR return value, because we don't log
messages at a level that exits non-locally (and that concept doesn't
even exist for FE logging).
Attachments:
v3-0001-Skip-unnecessary-stat-calls-in-walkdir.patchtext/x-patch; charset=UTF-8; name=v3-0001-Skip-unnecessary-stat-calls-in-walkdir.patchDownload
From 6f937ccef54afdcebaa52a036591523c67ac9d41 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Thu, 3 Sep 2020 13:58:17 +1200
Subject: [PATCH v3 1/3] Skip unnecessary stat() calls in walkdir().
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Some kernels can tell us the type of a "dirent", so we can avoid a call
to stat() or lstat() in many cases. In order to be able to apply this
change to both frontend and backend versions of walkdir(), define a new
function get_dirent_type() in a new translation unit file_utils_febe.c.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
Discussion: https://postgr.es/m/CA%2BhUKG%2BFzxupGGN4GpUdbzZN%2Btn6FQPHo8w0Q%2BAPH5Wz8RG%2Bww%40mail.gmail.com
---
src/backend/storage/file/fd.c | 33 ++++++-----
src/common/Makefile | 1 +
src/common/file_utils.c | 32 +++++------
src/common/file_utils_febe.c | 99 ++++++++++++++++++++++++++++++++
src/include/common/file_utils.h | 22 ++++++-
src/tools/msvc/Mkvcbuild.pm | 2 +-
src/tools/pgindent/typedefs.list | 1 +
7 files changed, 154 insertions(+), 36 deletions(-)
create mode 100644 src/common/file_utils_febe.c
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index f376a97ed6..bd72a87ee3 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -89,6 +89,7 @@
#include "access/xlog.h"
#include "catalog/pg_tablespace.h"
#include "common/file_perm.h"
+#include "common/file_utils.h"
#include "miscadmin.h"
#include "pgstat.h"
#include "portability/mem.h"
@@ -3340,8 +3341,6 @@ walkdir(const char *path,
while ((de = ReadDirExtended(dir, path, elevel)) != NULL)
{
char subpath[MAXPGPATH * 2];
- struct stat fst;
- int sret;
CHECK_FOR_INTERRUPTS();
@@ -3351,23 +3350,23 @@ walkdir(const char *path,
snprintf(subpath, sizeof(subpath), "%s/%s", path, de->d_name);
- if (process_symlinks)
- sret = stat(subpath, &fst);
- else
- sret = lstat(subpath, &fst);
-
- if (sret < 0)
+ switch (get_dirent_type(subpath, de, process_symlinks, elevel))
{
- ereport(elevel,
- (errcode_for_file_access(),
- errmsg("could not stat file \"%s\": %m", subpath)));
- continue;
- }
+ case PGFILETYPE_REG:
+ (*action) (subpath, false, elevel);
+ break;
+ case PGFILETYPE_DIR:
+ walkdir(subpath, action, false, elevel);
+ break;
+ default:
- if (S_ISREG(fst.st_mode))
- (*action) (subpath, false, elevel);
- else if (S_ISDIR(fst.st_mode))
- walkdir(subpath, action, false, elevel);
+ /*
+ * Errors are already reported directly by get_dirent_type(),
+ * and any remaining symlinks and unknown file types are
+ * ignored.
+ */
+ break;
+ }
}
FreeDir(dir); /* we ignore any error here */
diff --git a/src/common/Makefile b/src/common/Makefile
index 16619e4ba8..aac92aabe1 100644
--- a/src/common/Makefile
+++ b/src/common/Makefile
@@ -56,6 +56,7 @@ OBJS_COMMON = \
exec.o \
f2s.o \
file_perm.o \
+ file_utils_febe.o \
hashfn.o \
ip.o \
jsonapi.o \
diff --git a/src/common/file_utils.c b/src/common/file_utils.c
index a2faafdf13..e24f31dd3b 100644
--- a/src/common/file_utils.c
+++ b/src/common/file_utils.c
@@ -2,7 +2,7 @@
*
* File-processing utility routines.
*
- * Assorted utility functions to work on files.
+ * Assorted utility functions to work on files, frontend only.
*
*
* Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
@@ -167,8 +167,6 @@ walkdir(const char *path,
while (errno = 0, (de = readdir(dir)) != NULL)
{
char subpath[MAXPGPATH * 2];
- struct stat fst;
- int sret;
if (strcmp(de->d_name, ".") == 0 ||
strcmp(de->d_name, "..") == 0)
@@ -176,21 +174,23 @@ walkdir(const char *path,
snprintf(subpath, sizeof(subpath), "%s/%s", path, de->d_name);
- if (process_symlinks)
- sret = stat(subpath, &fst);
- else
- sret = lstat(subpath, &fst);
-
- if (sret < 0)
+ switch (get_dirent_type(subpath, de, process_symlinks, PG_LOG_ERROR))
{
- pg_log_error("could not stat file \"%s\": %m", subpath);
- continue;
+ case PGFILETYPE_REG:
+ (*action) (subpath, false);
+ break;
+ case PGFILETYPE_DIR:
+ walkdir(subpath, action, false);
+ break;
+ default:
+
+ /*
+ * Errors are already reported directly by get_dirent_type(),
+ * and any remaining symlinks and unknown file types are
+ * ignored.
+ */
+ break;
}
-
- if (S_ISREG(fst.st_mode))
- (*action) (subpath, false);
- else if (S_ISDIR(fst.st_mode))
- walkdir(subpath, action, false);
}
if (errno)
diff --git a/src/common/file_utils_febe.c b/src/common/file_utils_febe.c
new file mode 100644
index 0000000000..3c00ab7c4d
--- /dev/null
+++ b/src/common/file_utils_febe.c
@@ -0,0 +1,99 @@
+/*-------------------------------------------------------------------------
+ *
+ * File-processing utility routines.
+ *
+ * Assorted utility functions to work on files, frontend and backend.
+ *
+ *
+ * Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/common/file_utils_febe.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#ifdef FRONTEND
+#include "postgres_fe.h"
+#else
+#include "postgres.h"
+#endif
+
+#include <dirent.h>
+#include <sys/stat.h>
+
+#include "common/file_utils.h"
+#ifdef FRONTEND
+#include "common/logging.h"
+#else
+#include "utils/elog.h"
+#endif
+
+/*
+ * Return the type of a directory entry.
+ *
+ * In frontend code, elevel should be a level from logging.h; in backend code
+ * it should be an error level from elog.h.
+ */
+PGFileType
+get_dirent_type(const char *path,
+ const struct dirent *de,
+ bool look_through_symlinks,
+ int elevel)
+{
+ PGFileType result;
+
+ /*
+ * We want to know the type of a directory entry. Some systems tell us
+ * that directly in the dirent struct, but that's a BSD/GNU extension.
+ * Even when the interface is present, sometimes the type is unknown,
+ * depending on the filesystem in use or in some cases options used at
+ * filesystem creation time.
+ */
+#if defined(DT_UNKNOWN) && defined(DT_REG) && defined(DT_DIR) && defined(DT_LNK)
+ if (de->d_type == DT_REG)
+ result = PGFILETYPE_REG;
+ else if (de->d_type == DT_DIR)
+ result = PGFILETYPE_DIR;
+ else if (de->d_type == DT_LNK && !look_through_symlinks)
+ result = PGFILETYPE_LNK;
+ else
+ result = PGFILETYPE_UNKNOWN;
+#else
+ result = PGFILETYPE_UNKNOWN;
+#endif
+
+ if (result == PGFILETYPE_UNKNOWN)
+ {
+ struct stat fst;
+ int sret;
+
+
+ if (look_through_symlinks)
+ sret = stat(path, &fst);
+ else
+ sret = lstat(path, &fst);
+
+ if (sret < 0)
+ {
+ result = PGFILETYPE_ERROR;
+#ifdef FRONTEND
+ pg_log_generic(elevel, "could not stat file \"%s\": %m", path);
+#else
+ ereport(elevel,
+ (errcode_for_file_access(),
+ errmsg("could not stat file \"%s\": %m", path)));
+#endif
+ }
+ else if (S_ISREG(fst.st_mode))
+ result = PGFILETYPE_REG;
+ else if (S_ISDIR(fst.st_mode))
+ result = PGFILETYPE_DIR;
+#ifdef S_ISLNK
+ else if (S_ISLNK(fst.st_mode))
+ result = PGFILETYPE_LNK;
+#endif
+ }
+
+ return result;
+}
diff --git a/src/include/common/file_utils.h b/src/include/common/file_utils.h
index a7add75efa..16c7e7e249 100644
--- a/src/include/common/file_utils.h
+++ b/src/include/common/file_utils.h
@@ -1,6 +1,4 @@
/*-------------------------------------------------------------------------
- *
- * File-processing utility routines for frontend code
*
* Assorted utility functions to work on files.
*
@@ -15,10 +13,30 @@
#ifndef FILE_UTILS_H
#define FILE_UTILS_H
+#include <dirent.h>
+
+typedef enum PGFileType
+{
+ PGFILETYPE_ERROR,
+ PGFILETYPE_UNKNOWN,
+ PGFILETYPE_REG,
+ PGFILETYPE_DIR,
+ PGFILETYPE_LNK
+} PGFileType;
+
+/* Functions defined in file_utils_febe.c for both FE and BE code. */
+extern PGFileType get_dirent_type(const char *path,
+ const struct dirent *de,
+ bool look_through_symlinks,
+ int elevel);
+
+/* Functions defined in file_utils.c only for FE code. */
+#ifdef FRONTEND
extern int fsync_fname(const char *fname, bool isdir);
extern void fsync_pgdata(const char *pg_data, int serverVersion);
extern void fsync_dir_recurse(const char *dir);
extern int durable_rename(const char *oldfile, const char *newfile);
extern int fsync_parent_path(const char *fname);
+#endif
#endif /* FILE_UTILS_H */
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 20da7985c1..a64127a196 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -121,7 +121,7 @@ sub mkvcbuild
our @pgcommonallfiles = qw(
archive.c base64.c checksum_helper.c
config_info.c controldata_utils.c d2s.c encnames.c exec.c
- f2s.c file_perm.c hashfn.c ip.c jsonapi.c
+ f2s.c file_perm.c file_utils_febe.c hashfn.c ip.c jsonapi.c
keywords.c kwlookup.c link-canary.c md5.c
pg_lzcompress.c pgfnames.c psprintf.c relpath.c rmtree.c
saslprep.c scram-common.c string.c stringinfo.c unicode_norm.c username.c
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 3d990463ce..b4d40dda16 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1514,6 +1514,7 @@ PGEventResultCopy
PGEventResultCreate
PGEventResultDestroy
PGFInfoFunction
+PGFileType
PGFunction
PGLZ_HistEntry
PGLZ_Strategy
--
2.20.1
v3-0002-Add-d_type-to-Win32-dirent-port.patchtext/x-patch; charset=UTF-8; name=v3-0002-Add-d_type-to-Win32-dirent-port.patchDownload
From 616eba62a637b9b7440680d9ca4490d36347be46 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Thu, 3 Sep 2020 13:09:52 +1200
Subject: [PATCH v3 2/3] Add d_type to Win32 dirent port.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The member d_type is a BSD/GNU extension to struct dirent, but Windows
has the information required to populate it so let's add it to our
emulation so that get_dirent_type() can use it.
Author: Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
Discussion: https://postgr.es/m/CA%2BhUKG%2BFzxupGGN4GpUdbzZN%2Btn6FQPHo8w0Q%2BAPH5Wz8RG%2Bww%40mail.gmail.com
---
src/include/port/win32_msvc/dirent.h | 23 +++++++++++++++++++++++
src/port/dirent.c | 15 +++++++++++++++
2 files changed, 38 insertions(+)
diff --git a/src/include/port/win32_msvc/dirent.h b/src/include/port/win32_msvc/dirent.h
index 9fabdf332b..6a54f964f5 100644
--- a/src/include/port/win32_msvc/dirent.h
+++ b/src/include/port/win32_msvc/dirent.h
@@ -10,6 +10,7 @@ struct dirent
{
long d_ino;
unsigned short d_reclen;
+ unsigned char d_type;
unsigned short d_namlen;
char d_name[MAX_PATH];
};
@@ -20,4 +21,26 @@ DIR *opendir(const char *);
struct dirent *readdir(DIR *);
int closedir(DIR *);
+/* File types for 'd_type'. */
+enum
+ {
+ DT_UNKNOWN = 0,
+# define DT_UNKNOWN DT_UNKNOWN
+ DT_FIFO = 1,
+# define DT_FIFO DT_FIFO
+ DT_CHR = 2,
+# define DT_CHR DT_CHR
+ DT_DIR = 4,
+# define DT_DIR DT_DIR
+ DT_BLK = 6,
+# define DT_BLK DT_BLK
+ DT_REG = 8,
+# define DT_REG DT_REG
+ DT_LNK = 10,
+# define DT_LNK DT_LNK
+ DT_SOCK = 12,
+# define DT_SOCK DT_SOCK
+ DT_WHT = 14
+# define DT_WHT DT_WHT
+ };
#endif
diff --git a/src/port/dirent.c b/src/port/dirent.c
index b264484fca..519dd82101 100644
--- a/src/port/dirent.c
+++ b/src/port/dirent.c
@@ -69,6 +69,7 @@ opendir(const char *dirname)
d->handle = INVALID_HANDLE_VALUE;
d->ret.d_ino = 0; /* no inodes on win32 */
d->ret.d_reclen = 0; /* not used on win32 */
+ d->ret.d_type = DT_UNKNOWN;
return d;
}
@@ -77,6 +78,7 @@ struct dirent *
readdir(DIR *d)
{
WIN32_FIND_DATA fd;
+ DWORD attrib;
if (d->handle == INVALID_HANDLE_VALUE)
{
@@ -105,6 +107,19 @@ readdir(DIR *d)
}
strcpy(d->ret.d_name, fd.cFileName); /* Both strings are MAX_PATH long */
d->ret.d_namlen = strlen(d->ret.d_name);
+ /*
+ * The only identifed types are: directory, regular file or symbolic link.
+ * Errors are treated as a file type that could not be determined.
+ */
+ attrib = GetFileAttributes(d->ret.d_name);
+ if (attrib == INVALID_FILE_ATTRIBUTES)
+ d->ret.d_type = DT_UNKNOWN;
+ else if ((attrib & FILE_ATTRIBUTE_DIRECTORY) != 0)
+ d->ret.d_type = DT_DIR;
+ else if ((attrib & FILE_ATTRIBUTE_REPARSE_POINT) != 0)
+ d->ret.d_type = DT_LNK;
+ else
+ d->ret.d_type = DT_REG;
return &d->ret;
}
--
2.20.1
On 2020-Sep-04, Thomas Munro wrote:
@@ -10,6 +10,7 @@ struct dirent
{
long d_ino;
unsigned short d_reclen;
+ unsigned char d_type;
unsigned short d_namlen;
char d_name[MAX_PATH];
};
@@ -20,4 +21,26 @@ DIR *opendir(const char *);
struct dirent *readdir(DIR *);
int closedir(DIR *);+/* File types for 'd_type'. */ +enum + { + DT_UNKNOWN = 0, +# define DT_UNKNOWN DT_UNKNOWN
Uhm ... what do these #defines do? They look a bit funny.
Would it make sense to give this enum a name, and then use that name in
struct dirent's definition, instead of unsigned char?
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Sep 4, 2020 at 9:37 PM Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:
On 2020-Sep-04, Thomas Munro wrote:
@@ -10,6 +10,7 @@ struct dirent
{
long d_ino;
unsigned short d_reclen;
+ unsigned char d_type;
unsigned short d_namlen;
char d_name[MAX_PATH];
};
@@ -20,4 +21,26 @@ DIR *opendir(const char *);
struct dirent *readdir(DIR *);
int closedir(DIR *);+/* File types for 'd_type'. */ +enum + { + DT_UNKNOWN = 0, +# define DT_UNKNOWN DT_UNKNOWNUhm ... what do these #defines do? They look a bit funny.
Would it make sense to give this enum a name, and then use that name in
struct dirent's definition, instead of unsigned char?
They mimic POSIX dirent.h. I would rather stick to that.
Regards,
Juan José Santamaría Flecha
On 2020-Sep-04, Juan Jos� Santamar�a Flecha wrote:
On Fri, Sep 4, 2020 at 9:37 PM Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:On 2020-Sep-04, Thomas Munro wrote:
+/* File types for 'd_type'. */ +enum + { + DT_UNKNOWN = 0, +# define DT_UNKNOWN DT_UNKNOWNUhm ... what do these #defines do? They look a bit funny.
Would it make sense to give this enum a name, and then use that name in
struct dirent's definition, instead of unsigned char?They mimic POSIX dirent.h. I would rather stick to that.
Ah ... they do?
If you remove the #define lines, what happens to your patch?
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Sep 4, 2020 at 10:28 PM Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:
On 2020-Sep-04, Juan José Santamaría Flecha wrote:
On Fri, Sep 4, 2020 at 9:37 PM Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:On 2020-Sep-04, Thomas Munro wrote:
+/* File types for 'd_type'. */ +enum + { + DT_UNKNOWN = 0, +# define DT_UNKNOWN DT_UNKNOWNUhm ... what do these #defines do? They look a bit funny.
Would it make sense to give this enum a name, and then use that name in
struct dirent's definition, instead of unsigned char?They mimic POSIX dirent.h. I would rather stick to that.
Ah ... they do?
If you remove the #define lines, what happens to your patch?
If will fail to detect that the patch makes the optimisation available for
WIN32:
+#if defined(DT_UNKNOWN) && defined(DT_REG) && defined(DT_DIR) &&
defined(DT_LNK)
Regards,
Juan José Santamaría Flecha
On 2020-Sep-04, Juan Jos� Santamar�a Flecha wrote:
If will fail to detect that the patch makes the optimisation available for
WIN32:+#if defined(DT_UNKNOWN) && defined(DT_REG) && defined(DT_DIR) &&
defined(DT_LNK)
Oh, I see. I suggest that it'd be better to change this line instead.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
On 2020-Sep-04, Juan José Santamaría Flecha wrote:
If will fail to detect that the patch makes the optimisation available for
WIN32:+#if defined(DT_UNKNOWN) && defined(DT_REG) && defined(DT_DIR) &&
defined(DT_LNK)
Oh, I see. I suggest that it'd be better to change this line instead.
I think that it's standard to test for such symbols by seeing
if they're defined as macros ... not least because that's the *only*
way to test their existence in C.
Personally, what I'd do is lose the enum and just define the macros
with simple integer constant values.
regards, tom lane
Hi,
On 2020-09-02 17:51:27 +0200, Juan Jos� Santamar�a Flecha wrote:
Win32 could also benefit from this micro-optimisation if we expanded the
dirent port to include d_type. Please find attached a patch that does
so
.
} strcpy(d->ret.d_name, fd.cFileName); /* Both strings are MAX_PATH long */ d->ret.d_namlen = strlen(d->ret.d_name); + /* + * The only identifed types are: directory, regular file or symbolic link. + * Errors are treated as a file type that could not be determined. + */ + attrib = GetFileAttributes(d->ret.d_name); + if (attrib == INVALID_FILE_ATTRIBUTES) + d->ret.d_type = DT_UNKNOWN; + else if ((attrib & FILE_ATTRIBUTE_DIRECTORY) != 0) + d->ret.d_type = DT_DIR; + else if ((attrib & FILE_ATTRIBUTE_REPARSE_POINT) != 0) + d->ret.d_type = DT_LNK; + else + d->ret.d_type = DT_REG;return &d->ret;
}
Is this really an optimization? The benefit of Thomas' patch is that
that information sometimes already is there. But here you're doing a
separate lookup with GetFileAttributes()?
What am I missing?
Greetings,
Andres Freund
On 2020-Sep-04, Tom Lane wrote:
I think that it's standard to test for such symbols by seeing
if they're defined as macros ... not least because that's the *only*
way to test their existence in C.
I guess since what we're doing is emulating standard readdir(), that
makes sense.
Personally, what I'd do is lose the enum and just define the macros
with simple integer constant values.
WFM.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, Sep 5, 2020 at 9:45 AM Andres Freund <andres@anarazel.de> wrote:
On 2020-09-02 17:51:27 +0200, Juan José Santamaría Flecha wrote:
+ attrib = GetFileAttributes(d->ret.d_name);
Is this really an optimization? The benefit of Thomas' patch is that
that information sometimes already is there. But here you're doing a
separate lookup with GetFileAttributes()?
Well as discussed already, our stat() emulation on Windows does
multiple syscalls, so it's a slight improvement. However, it looks
like we might be missing a further opportunity here... Doesn't
Windows already give us the flags we need in the dwFileAttributes
member of the WIN32_FIND_DATA object that the Find{First,Next}File()
functions populate?
Hi,
On 2020-09-05 11:15:07 +1200, Thomas Munro wrote:
On Sat, Sep 5, 2020 at 9:45 AM Andres Freund <andres@anarazel.de> wrote:
On 2020-09-02 17:51:27 +0200, Juan Jos� Santamar�a Flecha wrote:
+ attrib = GetFileAttributes(d->ret.d_name);
Is this really an optimization? The benefit of Thomas' patch is that
that information sometimes already is there. But here you're doing a
separate lookup with GetFileAttributes()?Well as discussed already, our stat() emulation on Windows does
multiple syscalls, so it's a slight improvement.
But the patch is patching readdir(), not just walkdir(). Not all
readdir() / ReadDir() callers necessarily do a stat() and many continue
to do a stat() after the patches. So for all of those except walkdir(),
some like RemoveOldXlogFiles() sometimes being noticable cost wise, the
patch will increase the cost on windows. No? That's quite different
from utilizing "free" information.
However, it looks like we might be missing a further opportunity
here... Doesn't Windows already give us the flags we need in the
dwFileAttributes member of the WIN32_FIND_DATA object that the
Find{First,Next}File() functions populate?
That'd be better...
Greetings,
Andres Freund
On Sat, Sep 5, 2020 at 2:13 AM Andres Freund <andres@anarazel.de> wrote:
However, it looks like we might be missing a further opportunity
here... Doesn't Windows already give us the flags we need in the
dwFileAttributes member of the WIN32_FIND_DATA object that the
Find{First,Next}File() functions populate?That'd be better...
At first I did not see how to get DT_LNK directly, but it is possible
without additional calls, so please find attached a version with that logic.
This version also drops the enum, defining just the macros.
Regards,
Juan José Santamaría Flecha
Attachments:
v4-0001-Skip-unnecessary-stat-calls-in-walkdir.patchapplication/octet-stream; name=v4-0001-Skip-unnecessary-stat-calls-in-walkdir.patchDownload
From 6f937ccef54afdcebaa52a036591523c67ac9d41 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Thu, 3 Sep 2020 13:58:17 +1200
Subject: [PATCH v3 1/3] Skip unnecessary stat() calls in walkdir().
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Some kernels can tell us the type of a "dirent", so we can avoid a call
to stat() or lstat() in many cases. In order to be able to apply this
change to both frontend and backend versions of walkdir(), define a new
function get_dirent_type() in a new translation unit file_utils_febe.c.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
Discussion: https://postgr.es/m/CA%2BhUKG%2BFzxupGGN4GpUdbzZN%2Btn6FQPHo8w0Q%2BAPH5Wz8RG%2Bww%40mail.gmail.com
---
src/backend/storage/file/fd.c | 33 ++++++-----
src/common/Makefile | 1 +
src/common/file_utils.c | 32 +++++------
src/common/file_utils_febe.c | 99 ++++++++++++++++++++++++++++++++
src/include/common/file_utils.h | 22 ++++++-
src/tools/msvc/Mkvcbuild.pm | 2 +-
src/tools/pgindent/typedefs.list | 1 +
7 files changed, 154 insertions(+), 36 deletions(-)
create mode 100644 src/common/file_utils_febe.c
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index f376a97ed6..bd72a87ee3 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -89,6 +89,7 @@
#include "access/xlog.h"
#include "catalog/pg_tablespace.h"
#include "common/file_perm.h"
+#include "common/file_utils.h"
#include "miscadmin.h"
#include "pgstat.h"
#include "portability/mem.h"
@@ -3340,8 +3341,6 @@ walkdir(const char *path,
while ((de = ReadDirExtended(dir, path, elevel)) != NULL)
{
char subpath[MAXPGPATH * 2];
- struct stat fst;
- int sret;
CHECK_FOR_INTERRUPTS();
@@ -3351,23 +3350,23 @@ walkdir(const char *path,
snprintf(subpath, sizeof(subpath), "%s/%s", path, de->d_name);
- if (process_symlinks)
- sret = stat(subpath, &fst);
- else
- sret = lstat(subpath, &fst);
-
- if (sret < 0)
+ switch (get_dirent_type(subpath, de, process_symlinks, elevel))
{
- ereport(elevel,
- (errcode_for_file_access(),
- errmsg("could not stat file \"%s\": %m", subpath)));
- continue;
- }
+ case PGFILETYPE_REG:
+ (*action) (subpath, false, elevel);
+ break;
+ case PGFILETYPE_DIR:
+ walkdir(subpath, action, false, elevel);
+ break;
+ default:
- if (S_ISREG(fst.st_mode))
- (*action) (subpath, false, elevel);
- else if (S_ISDIR(fst.st_mode))
- walkdir(subpath, action, false, elevel);
+ /*
+ * Errors are already reported directly by get_dirent_type(),
+ * and any remaining symlinks and unknown file types are
+ * ignored.
+ */
+ break;
+ }
}
FreeDir(dir); /* we ignore any error here */
diff --git a/src/common/Makefile b/src/common/Makefile
index 16619e4ba8..aac92aabe1 100644
--- a/src/common/Makefile
+++ b/src/common/Makefile
@@ -56,6 +56,7 @@ OBJS_COMMON = \
exec.o \
f2s.o \
file_perm.o \
+ file_utils_febe.o \
hashfn.o \
ip.o \
jsonapi.o \
diff --git a/src/common/file_utils.c b/src/common/file_utils.c
index a2faafdf13..e24f31dd3b 100644
--- a/src/common/file_utils.c
+++ b/src/common/file_utils.c
@@ -2,7 +2,7 @@
*
* File-processing utility routines.
*
- * Assorted utility functions to work on files.
+ * Assorted utility functions to work on files, frontend only.
*
*
* Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
@@ -167,8 +167,6 @@ walkdir(const char *path,
while (errno = 0, (de = readdir(dir)) != NULL)
{
char subpath[MAXPGPATH * 2];
- struct stat fst;
- int sret;
if (strcmp(de->d_name, ".") == 0 ||
strcmp(de->d_name, "..") == 0)
@@ -176,21 +174,23 @@ walkdir(const char *path,
snprintf(subpath, sizeof(subpath), "%s/%s", path, de->d_name);
- if (process_symlinks)
- sret = stat(subpath, &fst);
- else
- sret = lstat(subpath, &fst);
-
- if (sret < 0)
+ switch (get_dirent_type(subpath, de, process_symlinks, PG_LOG_ERROR))
{
- pg_log_error("could not stat file \"%s\": %m", subpath);
- continue;
+ case PGFILETYPE_REG:
+ (*action) (subpath, false);
+ break;
+ case PGFILETYPE_DIR:
+ walkdir(subpath, action, false);
+ break;
+ default:
+
+ /*
+ * Errors are already reported directly by get_dirent_type(),
+ * and any remaining symlinks and unknown file types are
+ * ignored.
+ */
+ break;
}
-
- if (S_ISREG(fst.st_mode))
- (*action) (subpath, false);
- else if (S_ISDIR(fst.st_mode))
- walkdir(subpath, action, false);
}
if (errno)
diff --git a/src/common/file_utils_febe.c b/src/common/file_utils_febe.c
new file mode 100644
index 0000000000..3c00ab7c4d
--- /dev/null
+++ b/src/common/file_utils_febe.c
@@ -0,0 +1,99 @@
+/*-------------------------------------------------------------------------
+ *
+ * File-processing utility routines.
+ *
+ * Assorted utility functions to work on files, frontend and backend.
+ *
+ *
+ * Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/common/file_utils_febe.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#ifdef FRONTEND
+#include "postgres_fe.h"
+#else
+#include "postgres.h"
+#endif
+
+#include <dirent.h>
+#include <sys/stat.h>
+
+#include "common/file_utils.h"
+#ifdef FRONTEND
+#include "common/logging.h"
+#else
+#include "utils/elog.h"
+#endif
+
+/*
+ * Return the type of a directory entry.
+ *
+ * In frontend code, elevel should be a level from logging.h; in backend code
+ * it should be an error level from elog.h.
+ */
+PGFileType
+get_dirent_type(const char *path,
+ const struct dirent *de,
+ bool look_through_symlinks,
+ int elevel)
+{
+ PGFileType result;
+
+ /*
+ * We want to know the type of a directory entry. Some systems tell us
+ * that directly in the dirent struct, but that's a BSD/GNU extension.
+ * Even when the interface is present, sometimes the type is unknown,
+ * depending on the filesystem in use or in some cases options used at
+ * filesystem creation time.
+ */
+#if defined(DT_UNKNOWN) && defined(DT_REG) && defined(DT_DIR) && defined(DT_LNK)
+ if (de->d_type == DT_REG)
+ result = PGFILETYPE_REG;
+ else if (de->d_type == DT_DIR)
+ result = PGFILETYPE_DIR;
+ else if (de->d_type == DT_LNK && !look_through_symlinks)
+ result = PGFILETYPE_LNK;
+ else
+ result = PGFILETYPE_UNKNOWN;
+#else
+ result = PGFILETYPE_UNKNOWN;
+#endif
+
+ if (result == PGFILETYPE_UNKNOWN)
+ {
+ struct stat fst;
+ int sret;
+
+
+ if (look_through_symlinks)
+ sret = stat(path, &fst);
+ else
+ sret = lstat(path, &fst);
+
+ if (sret < 0)
+ {
+ result = PGFILETYPE_ERROR;
+#ifdef FRONTEND
+ pg_log_generic(elevel, "could not stat file \"%s\": %m", path);
+#else
+ ereport(elevel,
+ (errcode_for_file_access(),
+ errmsg("could not stat file \"%s\": %m", path)));
+#endif
+ }
+ else if (S_ISREG(fst.st_mode))
+ result = PGFILETYPE_REG;
+ else if (S_ISDIR(fst.st_mode))
+ result = PGFILETYPE_DIR;
+#ifdef S_ISLNK
+ else if (S_ISLNK(fst.st_mode))
+ result = PGFILETYPE_LNK;
+#endif
+ }
+
+ return result;
+}
diff --git a/src/include/common/file_utils.h b/src/include/common/file_utils.h
index a7add75efa..16c7e7e249 100644
--- a/src/include/common/file_utils.h
+++ b/src/include/common/file_utils.h
@@ -1,6 +1,4 @@
/*-------------------------------------------------------------------------
- *
- * File-processing utility routines for frontend code
*
* Assorted utility functions to work on files.
*
@@ -15,10 +13,30 @@
#ifndef FILE_UTILS_H
#define FILE_UTILS_H
+#include <dirent.h>
+
+typedef enum PGFileType
+{
+ PGFILETYPE_ERROR,
+ PGFILETYPE_UNKNOWN,
+ PGFILETYPE_REG,
+ PGFILETYPE_DIR,
+ PGFILETYPE_LNK
+} PGFileType;
+
+/* Functions defined in file_utils_febe.c for both FE and BE code. */
+extern PGFileType get_dirent_type(const char *path,
+ const struct dirent *de,
+ bool look_through_symlinks,
+ int elevel);
+
+/* Functions defined in file_utils.c only for FE code. */
+#ifdef FRONTEND
extern int fsync_fname(const char *fname, bool isdir);
extern void fsync_pgdata(const char *pg_data, int serverVersion);
extern void fsync_dir_recurse(const char *dir);
extern int durable_rename(const char *oldfile, const char *newfile);
extern int fsync_parent_path(const char *fname);
+#endif
#endif /* FILE_UTILS_H */
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 20da7985c1..a64127a196 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -121,7 +121,7 @@ sub mkvcbuild
our @pgcommonallfiles = qw(
archive.c base64.c checksum_helper.c
config_info.c controldata_utils.c d2s.c encnames.c exec.c
- f2s.c file_perm.c hashfn.c ip.c jsonapi.c
+ f2s.c file_perm.c file_utils_febe.c hashfn.c ip.c jsonapi.c
keywords.c kwlookup.c link-canary.c md5.c
pg_lzcompress.c pgfnames.c psprintf.c relpath.c rmtree.c
saslprep.c scram-common.c string.c stringinfo.c unicode_norm.c username.c
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 3d990463ce..b4d40dda16 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1514,6 +1514,7 @@ PGEventResultCopy
PGEventResultCreate
PGEventResultDestroy
PGFInfoFunction
+PGFileType
PGFunction
PGLZ_HistEntry
PGLZ_Strategy
--
2.20.1
v4-0002-Add-d_type-to-WIN32-dirent-port.patchapplication/octet-stream; name=v4-0002-Add-d_type-to-WIN32-dirent-port.patchDownload
diff --git a/src/include/port/win32_msvc/dirent.h b/src/include/port/win32_msvc/dirent.h
index 9fabdf3..d339242 100644
--- a/src/include/port/win32_msvc/dirent.h
+++ b/src/include/port/win32_msvc/dirent.h
@@ -10,6 +10,7 @@ struct dirent
{
long d_ino;
unsigned short d_reclen;
+ unsigned char d_type;
unsigned short d_namlen;
char d_name[MAX_PATH];
};
@@ -20,4 +21,14 @@ DIR *opendir(const char *);
struct dirent *readdir(DIR *);
int closedir(DIR *);
+/* File types for 'd_type'. */
+# define DT_UNKNOWN 0
+# define DT_FIFO 1
+# define DT_CHR 2
+# define DT_DIR 4
+# define DT_BLK 6
+# define DT_REG 8
+# define DT_LNK 10
+# define DT_SOCK 12
+# define DT_WHT 14
#endif
diff --git a/src/port/dirent.c b/src/port/dirent.c
index b264484..83db153 100644
--- a/src/port/dirent.c
+++ b/src/port/dirent.c
@@ -69,6 +69,7 @@ opendir(const char *dirname)
d->handle = INVALID_HANDLE_VALUE;
d->ret.d_ino = 0; /* no inodes on win32 */
d->ret.d_reclen = 0; /* not used on win32 */
+ d->ret.d_type = DT_UNKNOWN;
return d;
}
@@ -105,6 +106,15 @@ readdir(DIR *d)
}
strcpy(d->ret.d_name, fd.cFileName); /* Both strings are MAX_PATH long */
d->ret.d_namlen = strlen(d->ret.d_name);
+ /* The only identifed types are: directory, regular file or symbolic link */
+ if ((fd.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0)
+ d->ret.d_type = DT_DIR;
+ /* For reparse points dwReserved0 field will contain the ReparseTag */
+ else if ((fd.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) != 0
+ && (fd.dwReserved0 == IO_REPARSE_TAG_MOUNT_POINT))
+ d->ret.d_type = DT_LNK;
+ else
+ d->ret.d_type = DT_REG;
return &d->ret;
}
Hi Juan,
This is only a suggestion, if you find it appropriate.
We could use a little cut tail in get_dirent_type function.
Try to avoid add padding, when modifying or adding fields.
struct dirent
{
long d_ino;
unsigned short d_reclen;
unsigned short d_namlen;
+ unsigned char d_type;
char d_name[MAX_PATH];
};
Or even better if possible:
struct dirent
{
char d_name[MAX_PATH];
long d_ino;
unsigned short d_reclen;
unsigned short d_namlen;
unsigned char d_type;
};
regards,
Ranier Vilela
Attachments:
v4-0002-Add-d_type-to-WIN32-dirent-port.patchapplication/octet-stream; name=v4-0002-Add-d_type-to-WIN32-dirent-port.patchDownload
diff --git a/src/include/port/win32_msvc/dirent.h b/src/include/port/win32_msvc/dirent.h
index 9fabdf3..d339242 100644
--- a/src/include/port/win32_msvc/dirent.h
+++ b/src/include/port/win32_msvc/dirent.h
@@ -10,6 +10,7 @@ struct dirent
{
long d_ino;
unsigned short d_reclen;
unsigned short d_namlen;
+ unsigned char d_type;
char d_name[MAX_PATH];
};
@@ -20,4 +21,14 @@ DIR *opendir(const char *);
struct dirent *readdir(DIR *);
int closedir(DIR *);
+/* File types for 'd_type'. */
+# define DT_UNKNOWN 0
+# define DT_FIFO 1
+# define DT_CHR 2
+# define DT_DIR 4
+# define DT_BLK 6
+# define DT_REG 8
+# define DT_LNK 10
+# define DT_SOCK 12
+# define DT_WHT 14
#endif
diff --git a/src/port/dirent.c b/src/port/dirent.c
index b264484..83db153 100644
--- a/src/port/dirent.c
+++ b/src/port/dirent.c
@@ -69,6 +69,7 @@ opendir(const char *dirname)
d->handle = INVALID_HANDLE_VALUE;
d->ret.d_ino = 0; /* no inodes on win32 */
d->ret.d_reclen = 0; /* not used on win32 */
+ d->ret.d_type = DT_UNKNOWN;
return d;
}
@@ -105,6 +106,15 @@ readdir(DIR *d)
}
strcpy(d->ret.d_name, fd.cFileName); /* Both strings are MAX_PATH long */
d->ret.d_namlen = strlen(d->ret.d_name);
+ /* The only identifed types are: directory, regular file or symbolic link */
+ if ((fd.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0)
+ d->ret.d_type = DT_DIR;
+ /* For reparse points dwReserved0 field will contain the ReparseTag */
+ else if ((fd.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) != 0
+ && (fd.dwReserved0 == IO_REPARSE_TAG_MOUNT_POINT))
+ d->ret.d_type = DT_LNK;
+ else
+ d->ret.d_type = DT_REG;
return &d->ret;
}
v4-0001-Skip-unnecessary-stat-calls-in-walkdir.patchapplication/octet-stream; name=v4-0001-Skip-unnecessary-stat-calls-in-walkdir.patchDownload
From 6f937ccef54afdcebaa52a036591523c67ac9d41 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Thu, 3 Sep 2020 13:58:17 +1200
Subject: [PATCH v3 1/3] Skip unnecessary stat() calls in walkdir().
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Some kernels can tell us the type of a "dirent", so we can avoid a call
to stat() or lstat() in many cases. In order to be able to apply this
change to both frontend and backend versions of walkdir(), define a new
function get_dirent_type() in a new translation unit file_utils_febe.c.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
Discussion: https://postgr.es/m/CA%2BhUKG%2BFzxupGGN4GpUdbzZN%2Btn6FQPHo8w0Q%2BAPH5Wz8RG%2Bww%40mail.gmail.com
---
src/backend/storage/file/fd.c | 33 ++++++-----
src/common/Makefile | 1 +
src/common/file_utils.c | 32 +++++------
src/common/file_utils_febe.c | 99 ++++++++++++++++++++++++++++++++
src/include/common/file_utils.h | 22 ++++++-
src/tools/msvc/Mkvcbuild.pm | 2 +-
src/tools/pgindent/typedefs.list | 1 +
7 files changed, 154 insertions(+), 36 deletions(-)
create mode 100644 src/common/file_utils_febe.c
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index f376a97ed6..bd72a87ee3 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -89,6 +89,7 @@
#include "access/xlog.h"
#include "catalog/pg_tablespace.h"
#include "common/file_perm.h"
+#include "common/file_utils.h"
#include "miscadmin.h"
#include "pgstat.h"
#include "portability/mem.h"
@@ -3340,8 +3341,6 @@ walkdir(const char *path,
while ((de = ReadDirExtended(dir, path, elevel)) != NULL)
{
char subpath[MAXPGPATH * 2];
- struct stat fst;
- int sret;
CHECK_FOR_INTERRUPTS();
@@ -3351,23 +3350,23 @@ walkdir(const char *path,
snprintf(subpath, sizeof(subpath), "%s/%s", path, de->d_name);
- if (process_symlinks)
- sret = stat(subpath, &fst);
- else
- sret = lstat(subpath, &fst);
-
- if (sret < 0)
+ switch (get_dirent_type(subpath, de, process_symlinks, elevel))
{
- ereport(elevel,
- (errcode_for_file_access(),
- errmsg("could not stat file \"%s\": %m", subpath)));
- continue;
- }
+ case PGFILETYPE_REG:
+ (*action) (subpath, false, elevel);
+ break;
+ case PGFILETYPE_DIR:
+ walkdir(subpath, action, false, elevel);
+ break;
+ default:
- if (S_ISREG(fst.st_mode))
- (*action) (subpath, false, elevel);
- else if (S_ISDIR(fst.st_mode))
- walkdir(subpath, action, false, elevel);
+ /*
+ * Errors are already reported directly by get_dirent_type(),
+ * and any remaining symlinks and unknown file types are
+ * ignored.
+ */
+ break;
+ }
}
FreeDir(dir); /* we ignore any error here */
diff --git a/src/common/Makefile b/src/common/Makefile
index 16619e4ba8..aac92aabe1 100644
--- a/src/common/Makefile
+++ b/src/common/Makefile
@@ -56,6 +56,7 @@ OBJS_COMMON = \
exec.o \
f2s.o \
file_perm.o \
+ file_utils_febe.o \
hashfn.o \
ip.o \
jsonapi.o \
diff --git a/src/common/file_utils.c b/src/common/file_utils.c
index a2faafdf13..e24f31dd3b 100644
--- a/src/common/file_utils.c
+++ b/src/common/file_utils.c
@@ -2,7 +2,7 @@
*
* File-processing utility routines.
*
- * Assorted utility functions to work on files.
+ * Assorted utility functions to work on files, frontend only.
*
*
* Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
@@ -167,8 +167,6 @@ walkdir(const char *path,
while (errno = 0, (de = readdir(dir)) != NULL)
{
char subpath[MAXPGPATH * 2];
- struct stat fst;
- int sret;
if (strcmp(de->d_name, ".") == 0 ||
strcmp(de->d_name, "..") == 0)
@@ -176,21 +174,23 @@ walkdir(const char *path,
snprintf(subpath, sizeof(subpath), "%s/%s", path, de->d_name);
- if (process_symlinks)
- sret = stat(subpath, &fst);
- else
- sret = lstat(subpath, &fst);
-
- if (sret < 0)
+ switch (get_dirent_type(subpath, de, process_symlinks, PG_LOG_ERROR))
{
- pg_log_error("could not stat file \"%s\": %m", subpath);
- continue;
+ case PGFILETYPE_REG:
+ (*action) (subpath, false);
+ break;
+ case PGFILETYPE_DIR:
+ walkdir(subpath, action, false);
+ break;
+ default:
+
+ /*
+ * Errors are already reported directly by get_dirent_type(),
+ * and any remaining symlinks and unknown file types are
+ * ignored.
+ */
+ break;
}
-
- if (S_ISREG(fst.st_mode))
- (*action) (subpath, false);
- else if (S_ISDIR(fst.st_mode))
- walkdir(subpath, action, false);
}
if (errno)
diff --git a/src/common/file_utils_febe.c b/src/common/file_utils_febe.c
new file mode 100644
index 0000000000..3c00ab7c4d
--- /dev/null
+++ b/src/common/file_utils_febe.c
@@ -0,0 +1,99 @@
+/*-------------------------------------------------------------------------
+ *
+ * File-processing utility routines.
+ *
+ * Assorted utility functions to work on files, frontend and backend.
+ *
+ *
+ * Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/common/file_utils_febe.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#ifdef FRONTEND
+#include "postgres_fe.h"
+#else
+#include "postgres.h"
+#endif
+
+#include <dirent.h>
+#include <sys/stat.h>
+
+#include "common/file_utils.h"
+#ifdef FRONTEND
+#include "common/logging.h"
+#else
+#include "utils/elog.h"
+#endif
+
+/*
+ * Return the type of a directory entry.
+ *
+ * In frontend code, elevel should be a level from logging.h; in backend code
+ * it should be an error level from elog.h.
+ */
+PGFileType
+get_dirent_type(const char *path,
+ const struct dirent *de,
+ bool look_through_symlinks,
+ int elevel)
+{
+ struct stat fst;
+ int sret;
+
+ /*
+ * We want to know the type of a directory entry. Some systems tell us
+ * that directly in the dirent struct, but that's a BSD/GNU extension.
+ * Even when the interface is present, sometimes the type is unknown,
+ * depending on the filesystem in use or in some cases options used at
+ * filesystem creation time.
+ */
+#if defined(DT_UNKNOWN) && defined(DT_REG) && defined(DT_DIR) && defined(DT_LNK)
+ if (de->d_type == DT_REG)
+ return PGFILETYPE_REG;
+ else if (de->d_type == DT_DIR)
+ return PGFILETYPE_DIR;
+ else if (de->d_type == DT_LNK && !look_through_symlinks)
+ return PGFILETYPE_LNK;
+#endif
+
+ if (look_through_symlinks)
+ sret = stat(path, &fst);
+ else
+ sret = lstat(path, &fst);
+
+ if (sret < 0)
+ {
+#ifdef FRONTEND
+ pg_log_generic(elevel, "could not stat file \"%s\": %m", path);
+#else
+ ereport(elevel,
+ (errcode_for_file_access(),
+ errmsg("could not stat file \"%s\": %m", path)));
+#endif
+ return PGFILETYPE_ERROR;
+ }
+ else if (S_ISREG(fst.st_mode))
+ return PGFILETYPE_REG;
+ else if (S_ISDIR(fst.st_mode))
+ return PGFILETYPE_DIR;
+#ifdef S_ISLNK
+ else if (S_ISLNK(fst.st_mode))
+ return PGFILETYPE_LNK;
+#endif
+ }
+
+ return PGFILETYPE_UNKNOWN;
+}
diff --git a/src/include/common/file_utils.h b/src/include/common/file_utils.h
index a7add75efa..16c7e7e249 100644
--- a/src/include/common/file_utils.h
+++ b/src/include/common/file_utils.h
@@ -1,6 +1,4 @@
/*-------------------------------------------------------------------------
- *
- * File-processing utility routines for frontend code
*
* Assorted utility functions to work on files.
*
@@ -15,10 +13,30 @@
#ifndef FILE_UTILS_H
#define FILE_UTILS_H
+#include <dirent.h>
+
+typedef enum PGFileType
+{
+ PGFILETYPE_ERROR,
+ PGFILETYPE_UNKNOWN,
+ PGFILETYPE_REG,
+ PGFILETYPE_DIR,
+ PGFILETYPE_LNK
+} PGFileType;
+
+/* Functions defined in file_utils_febe.c for both FE and BE code. */
+extern PGFileType get_dirent_type(const char *path,
+ const struct dirent *de,
+ bool look_through_symlinks,
+ int elevel);
+
+/* Functions defined in file_utils.c only for FE code. */
+#ifdef FRONTEND
extern int fsync_fname(const char *fname, bool isdir);
extern void fsync_pgdata(const char *pg_data, int serverVersion);
extern void fsync_dir_recurse(const char *dir);
extern int durable_rename(const char *oldfile, const char *newfile);
extern int fsync_parent_path(const char *fname);
+#endif
#endif /* FILE_UTILS_H */
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 20da7985c1..a64127a196 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -121,7 +121,7 @@ sub mkvcbuild
our @pgcommonallfiles = qw(
archive.c base64.c checksum_helper.c
config_info.c controldata_utils.c d2s.c encnames.c exec.c
- f2s.c file_perm.c hashfn.c ip.c jsonapi.c
+ f2s.c file_perm.c file_utils_febe.c hashfn.c ip.c jsonapi.c
keywords.c kwlookup.c link-canary.c md5.c
pg_lzcompress.c pgfnames.c psprintf.c relpath.c rmtree.c
saslprep.c scram-common.c string.c stringinfo.c unicode_norm.c username.c
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 3d990463ce..b4d40dda16 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1514,6 +1514,7 @@ PGEventResultCopy
PGEventResultCreate
PGEventResultDestroy
PGFInfoFunction
+PGFileType
PGFunction
PGLZ_HistEntry
PGLZ_Strategy
--
2.20.1
Import Notes
Resolved by subject fallback
On Sun, Sep 6, 2020 at 5:23 AM Juan José Santamaría Flecha
<juanjo.santamaria@gmail.com> wrote:
On Sat, Sep 5, 2020 at 2:13 AM Andres Freund <andres@anarazel.de> wrote:
However, it looks like we might be missing a further opportunity
here... Doesn't Windows already give us the flags we need in the
dwFileAttributes member of the WIN32_FIND_DATA object that the
Find{First,Next}File() functions populate?That'd be better...
At first I did not see how to get DT_LNK directly, but it is possible without additional calls, so please find attached a version with that logic.
This version also drops the enum, defining just the macros.
Excellent. I'd like to commit these soon, unless someone has a better
idea for how to name file_utils_febe.c.
I think the following is a little mysterious, but it does seem to be
what people do for this in other projects. It is the documented way
to detect mount points, and I guess IO_REPARSE_TAG_MOUNT_POINT is
either overloaded also for junctions, or junctions are the same thing
as mount points. It would be nice to see a Win32 documentation page
that explicitly said that.
+ /* For reparse points dwReserved0 field will contain the ReparseTag */
+ else if ((fd.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) != 0
+ && (fd.dwReserved0 == IO_REPARSE_TAG_MOUNT_POINT))
+ d->ret.d_type = DT_LNK;Hmm, it's interesting that our existing test for a junction in
pgwin32_is_junction() only looks for FILE_ATTRIBUTE_REPARSE_POINT and
doesn't care what kind of reparse point it is.
Thomas Munro <thomas.munro@gmail.com> writes:
Excellent. I'd like to commit these soon, unless someone has a better
idea for how to name file_utils_febe.c.
As long as it's in src/port or src/common, isn't it implicit that
it's probably FE/BE common code?
I think it'd make more sense to insert all this stuff into file_utils.c,
and then just "#ifdef FRONTEND" the existing code there that doesn't work
in backend. For one thing, that gives us a saner upgrade path whenever
somebody gets ambitious enough to make that code work for the backend.
regards, tom lane
On Mon, Sep 7, 2020 at 10:36 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thomas Munro <thomas.munro@gmail.com> writes:
Excellent. I'd like to commit these soon, unless someone has a better
idea for how to name file_utils_febe.c.As long as it's in src/port or src/common, isn't it implicit that
it's probably FE/BE common code?I think it'd make more sense to insert all this stuff into file_utils.c,
and then just "#ifdef FRONTEND" the existing code there that doesn't work
in backend. For one thing, that gives us a saner upgrade path whenever
somebody gets ambitious enough to make that code work for the backend.
True. Ok, I committed the Unix patch that way. I know it works on
Linux, FreeBSD, macOS and Windows (without Juan José's patch), but
I'll have to check the build farm later to make sure HPUX, AIX and
Solaris are OK with this. It's remotely possible that one of them
defines DT_REG etc but doesn't have d_type; I'm hoping to get away
with not adding a configure check.
I'll wait a bit longer for comments on the Windows patch.
On Mon, Sep 7, 2020 at 12:27 AM Thomas Munro <thomas.munro@gmail.com> wrote:
On Sun, Sep 6, 2020 at 5:23 AM Juan José Santamaría Flecha
<juanjo.santamaria@gmail.com> wrote:On Sat, Sep 5, 2020 at 2:13 AM Andres Freund <andres@anarazel.de> wrote:
However, it looks like we might be missing a further opportunity
here... Doesn't Windows already give us the flags we need in the
dwFileAttributes member of the WIN32_FIND_DATA object that the
Find{First,Next}File() functions populate?That'd be better...
At first I did not see how to get DT_LNK directly, but it is possible
without additional calls, so please find attached a version with that logic.
This version also drops the enum, defining just the macros.
Excellent. I'd like to commit these soon, unless someone has a better
idea for how to name file_utils_febe.c.I think the following is a little mysterious, but it does seem to be
what people do for this in other projects. It is the documented way
to detect mount points, and I guess IO_REPARSE_TAG_MOUNT_POINT is
either overloaded also for junctions, or junctions are the same thing
as mount points. It would be nice to see a Win32 documentation page
that explicitly said that.
The wikipedia page on it is actually fairly decent:
https://en.wikipedia.org/wiki/NTFS_reparse_point. It's not the
documentation of course, but it's easier to read :) The core difference is
whether you mount a whole filesystem (mount point) or just a directory off
something mounted elsehwere (junction).
And yes, the wikipedia page confirms that junctions also use
IO_REPARSE_TAG_MOUNT_POINT.
+ /* For reparse points dwReserved0 field will contain the ReparseTag */
+ else if ((fd.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) != 0 + && (fd.dwReserved0 == IO_REPARSE_TAG_MOUNT_POINT)) + d->ret.d_type = DT_LNK;Hmm, it's interesting that our existing test for a junction in
pgwin32_is_junction() only looks for FILE_ATTRIBUTE_REPARSE_POINT and
doesn't care what kind of reparse point it is.
I think that's mostly historical. When that code was written, the only two
types of reparse points that existed were junctions and mount points --
which are as already noted, the same. Symbolic links, unix sockets and such
things came later.
--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>
On Mon, Sep 7, 2020 at 9:42 PM Magnus Hagander <magnus@hagander.net> wrote:
On Mon, Sep 7, 2020 at 12:27 AM Thomas Munro <thomas.munro@gmail.com> wrote:
I think the following is a little mysterious, but it does seem to be
what people do for this in other projects. It is the documented way
to detect mount points, and I guess IO_REPARSE_TAG_MOUNT_POINT is
either overloaded also for junctions, or junctions are the same thing
as mount points. It would be nice to see a Win32 documentation page
that explicitly said that.The wikipedia page on it is actually fairly decent: https://en.wikipedia.org/wiki/NTFS_reparse_point. It's not the documentation of course, but it's easier to read :) The core difference is whether you mount a whole filesystem (mount point) or just a directory off something mounted elsehwere (junction).
And yes, the wikipedia page confirms that junctions also use IO_REPARSE_TAG_MOUNT_POINT.
Thanks for confirming. I ran the Windows patch through pgindent,
fixed a small typo, and pushed.
On Mon, Sep 7, 2020 at 1:41 PM Thomas Munro <thomas.munro@gmail.com> wrote:
On Mon, Sep 7, 2020 at 9:42 PM Magnus Hagander <magnus@hagander.net>
wrote:On Mon, Sep 7, 2020 at 12:27 AM Thomas Munro <thomas.munro@gmail.com>
wrote:
I think the following is a little mysterious, but it does seem to be
what people do for this in other projects. It is the documented way
to detect mount points, and I guess IO_REPARSE_TAG_MOUNT_POINT is
either overloaded also for junctions, or junctions are the same thing
as mount points. It would be nice to see a Win32 documentation page
that explicitly said that.The wikipedia page on it is actually fairly decent:
https://en.wikipedia.org/wiki/NTFS_reparse_point. It's not the
documentation of course, but it's easier to read :) The core difference is
whether you mount a whole filesystem (mount point) or just a directory off
something mounted elsehwere (junction).And yes, the wikipedia page confirms that junctions also use
IO_REPARSE_TAG_MOUNT_POINT.
Thanks for confirming. I ran the Windows patch through pgindent,
fixed a small typo, and pushed.
Great, thanks. Should we include something from this discussion as comments?
Regards,
Juan José Santamaría Flecha
On Tue, Sep 8, 2020 at 2:05 AM Juan José Santamaría Flecha
<juanjo.santamaria@gmail.com> wrote:
On Mon, Sep 7, 2020 at 1:41 PM Thomas Munro <thomas.munro@gmail.com> wrote:
Thanks for confirming. I ran the Windows patch through pgindent,
fixed a small typo, and pushed.Great, thanks. Should we include something from this discussion as comments?
I dunno, it seems like this may be common knowledge for Windows people
and I was just being paranoid by asking for more info as a
non-Windowsian, but if you want to propose new comments or perhaps
just a pointer to one central place where we explain how that works, I
wouldn't be against that.
FWIW I just spotted a couple of very suspicious looking failures for
build farm animal "walleye", a "MinGW64 8.1.0" system, that say:
c:\\pgbuildfarm\\pgbuildroot\\HEAD\\pgsql.build\\src\\bin\\pg_upgrade>RMDIR
/s/q "c:\\pgbuildfarm\\pgbuildroot\\HEAD\\pgsql.build\\src\\bin\\pg_upgrade\\tmp_check\\data.old"
The directory is not empty.
Then I noticed it failed the same way 8 days ago, so I don't know
what's up with that but it looks like we didn't break it...
Thomas Munro <thomas.munro@gmail.com> writes:
FWIW I just spotted a couple of very suspicious looking failures for
build farm animal "walleye", a "MinGW64 8.1.0" system, that say:
walleye's been kind of unstable since the get-go, so I wouldn't put
too much faith in reports just from it.
regards, tom lane
On Tue, Sep 8, 2020 at 10:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thomas Munro <thomas.munro@gmail.com> writes:
FWIW I just spotted a couple of very suspicious looking failures for
build farm animal "walleye", a "MinGW64 8.1.0" system, that say:walleye's been kind of unstable since the get-go, so I wouldn't put
too much faith in reports just from it.
CC'ing animal owner.
<pokes at the logs> I suspect that someone who knows about PostgreSQL
on Windows would recognise the above symptom, but my guess is the
Windows "indexing" service is on, or an antivirus thing, or some other
kind of automatically-open-and-sniff-every-file-on-certain-file-events
thing. It looks like nothing of ours is even running at that moment
("waiting for server to shut down.... done"), and it's the RMDIR /s
shell command that is reporting the error. The other low probability
error seen on this host is this one:
+ERROR: could not stat file "pg_wal/000000010000000000000007":
Permission denied
On Mon, Sep 14, 2020 at 12:42 AM Thomas Munro <thomas.munro@gmail.com>
wrote:
On Tue, Sep 8, 2020 at 10:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thomas Munro <thomas.munro@gmail.com> writes:
FWIW I just spotted a couple of very suspicious looking failures for
build farm animal "walleye", a "MinGW64 8.1.0" system, that say:walleye's been kind of unstable since the get-go, so I wouldn't put
too much faith in reports just from it.CC'ing animal owner.
<pokes at the logs> I suspect that someone who knows about PostgreSQL
on Windows would recognise the above symptom, but my guess is the
Windows "indexing" service is on, or an antivirus thing, or some other
kind of automatically-open-and-sniff-every-file-on-certain-file-events
thing. It looks like nothing of ours is even running at that moment
("waiting for server to shut down.... done"), and it's the RMDIR /s
shell command that is reporting the error. The other low probability
error seen on this host is this one:+ERROR: could not stat file "pg_wal/000000010000000000000007":
Permission denied
This is a known problem, previous to this patch. There have been a couple
of attempts to tackle it, and hopefully the shared-memory based stats
collector will take care of it:
/messages/by-id/18986.1585585020@sss.pgh.pa.us
Regards,
Juan José Santamaría Flecha