Hi,
The comment for pgwin32_is_junction() says "Assumes the file exists,
so will return false if it doesn't (since a nonexistent file is not a
junction)". In fact that's the behaviour for any kind of error, and
although we set errno in that case, no caller ever checks it.
I think it'd be better to add missing_ok and elevel parameters,
following existing patterns. Unfortunately, it can't use the generic
frontend logging to implement elevel in frontend code from its current
location, because pgport can't call pgcommon. For now I came up with
a kludge to work around that problem, but I don't like it, and would
need to come up with something better...
Sketch code attached.
From 8e0e51359c0191cf673c3ddc87a3262b13f530a4 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Wed, 16 Mar 2022 23:05:39 +1300
Subject: [PATCH] Raise errors in pgwin32_is_junction().
XXX It's not very nice to use elevel like this in frontend code
---
src/backend/access/transam/xlog.c | 2 +-
src/backend/replication/basebackup.c | 4 ++--
src/backend/storage/file/fd.c | 2 +-
src/backend/utils/adt/misc.c | 2 +-
src/bin/pg_checksums/pg_checksums.c | 2 +-
src/bin/pg_rewind/file_ops.c | 2 +-
src/common/file_utils.c | 5 +++-
src/include/port.h | 2 +-
src/include/port/win32_port.h | 2 +-
src/port/dirmod.c | 36 +++++++++++++++++++++++++---
10 files changed, 46 insertions(+), 13 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 4ac3871c74..819b05177d 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8314,7 +8314,7 @@ do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p,
* directories directly under pg_tblspc, which would fail below.
*/
#ifdef WIN32
- if (!pgwin32_is_junction(fullpath))
+ if (!pgwin32_is_junction(fullpath, false, ERROR))
continue;
#else
if (get_dirent_type(fullpath, de, false, ERROR) != PGFILETYPE_LNK)
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index 6884cad2c0..4be9090445 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -1352,7 +1352,7 @@ sendDir(bbsink *sink, const char *path, int basepathlen, bool sizeonly,
#ifndef WIN32
S_ISLNK(statbuf.st_mode)
#else
- pgwin32_is_junction(pathbuf)
+ pgwin32_is_junction(pathbuf, true, ERROR)
#endif
)
{
@@ -1840,7 +1840,7 @@ convert_link_to_directory(const char *pathbuf, struct stat *statbuf)
#ifndef WIN32
if (S_ISLNK(statbuf->st_mode))
#else
- if (pgwin32_is_junction(pathbuf))
+ if (pgwin32_is_junction(pathbuf, true, ERROR))
#endif
statbuf->st_mode = S_IFDIR | pg_dir_create_mode;
}
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 14b77f2861..ec2f49fefa 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -3453,7 +3453,7 @@ SyncDataDirectory(void)
xlog_is_symlink = true;
}
#else
- if (pgwin32_is_junction("pg_wal"))
+ if (pgwin32_is_junction("pg_wal", false, LOG))
xlog_is_symlink = true;
#endif
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index 89690be2ed..723504ab10 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -317,7 +317,7 @@ pg_tablespace_location(PG_FUNCTION_ARGS)
* found, a relative path to the data directory is returned.
*/
#ifdef WIN32
- if (!pgwin32_is_junction(sourcepath))
+ if (!pgwin32_is_junction(sourcepath, false, ERROR))
PG_RETURN_TEXT_P(cstring_to_text(sourcepath));
#else
if (lstat(sourcepath, &st) < 0)
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 5f0f5ee62d..5720907d33 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -404,7 +404,7 @@ scan_directory(const char *basedir, const char *subdir, bool sizeonly)
#ifndef WIN32
else if (S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode))
#else
- else if (S_ISDIR(st.st_mode) || pgwin32_is_junction(fn))
+ else if (S_ISDIR(st.st_mode) || pgwin32_is_junction(fn, false, 1))
#endif
{
/*
diff --git a/src/bin/pg_rewind/file_ops.c b/src/bin/pg_rewind/file_ops.c
index 6cb288f099..1eda2baf2f 100644
--- a/src/bin/pg_rewind/file_ops.c
+++ b/src/bin/pg_rewind/file_ops.c
@@ -434,7 +434,7 @@ recurse_dir(const char *datadir, const char *parentpath,
#ifndef WIN32
else if (S_ISLNK(fst.st_mode))
#else
- else if (pgwin32_is_junction(fullpath))
+ else if (pgwin32_is_junction(fullpath, false, 1))
#endif
{
#if defined(HAVE_READLINK) || defined(WIN32)
diff --git a/src/common/file_utils.c b/src/common/file_utils.c
index 7138068633..2db909a3aa 100644
--- a/src/common/file_utils.c
+++ b/src/common/file_utils.c
@@ -88,8 +88,11 @@ fsync_pgdata(const char *pg_data,
else if (S_ISLNK(st.st_mode))
xlog_is_symlink = true;
}
+#elif defined(FRONTEND)
+ if (pgwin32_is_junction(pg_wal, false, 1))
+ xlog_is_symlink = true;
#else
- if (pgwin32_is_junction(pg_wal))
+ if (pgwin32_is_junction(pg_wal, false, ERROR))
xlog_is_symlink = true;
#endif
diff --git a/src/include/port.h b/src/include/port.h
index 3d103a2b31..b8118a3d51 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -284,7 +284,7 @@ extern int pgunlink(const char *path);
#if defined(WIN32) && !defined(__CYGWIN__)
extern int pgsymlink(const char *oldpath, const char *newpath);
extern int pgreadlink(const char *path, char *buf, size_t size);
-extern bool pgwin32_is_junction(const char *path);
+extern bool pgwin32_is_junction(const char *path, bool missing_ok, int elevel);
#define symlink(oldpath, newpath) pgsymlink(oldpath, newpath)
#define readlink(path, buf, size) pgreadlink(path, buf, size)
diff --git a/src/include/port/win32_port.h b/src/include/port/win32_port.h
index d3cb765976..d9bec24348 100644
--- a/src/include/port/win32_port.h
+++ b/src/include/port/win32_port.h
@@ -230,7 +230,7 @@ int setitimer(int which, const struct itimerval *value, struct itimerval *oval
*/
extern int pgsymlink(const char *oldpath, const char *newpath);
extern int pgreadlink(const char *path, char *buf, size_t size);
-extern bool pgwin32_is_junction(const char *path);
+extern bool pgwin32_is_junction(const char *path, bool missing_ok, int elevel);
#define symlink(oldpath, newpath) pgsymlink(oldpath, newpath)
#define readlink(path, buf, size) pgreadlink(path, buf, size)
diff --git a/src/port/dirmod.c b/src/port/dirmod.c
index 7ce042e75d..127cb6a576 100644
--- a/src/port/dirmod.c
+++ b/src/port/dirmod.c
@@ -337,19 +337,49 @@ pgreadlink(const char *path, char *buf, size_t size)
}
/*
- * Assumes the file exists, so will return false if it doesn't
- * (since a nonexistent file is not a junction)
+ * In frontend code, elevel must be 0 or 1, where 1 means write an error and
+ * exit(1); in backend code it should be 0 or a level from elog.h, to pass to
+ * ereport(). If elevel is 0, or missing_ok is set and the path doesn't exist,
+ * no logging is done. The caller should check errno on false return, unless
+ * elevel is set to a level that doesn't return on error.
*/
bool
-pgwin32_is_junction(const char *path)
+pgwin32_is_junction(const char *path, bool missing_ok, int elevel)
{
DWORD attr = GetFileAttributes(path);
+#ifdef FRONTEND
+ /*
+ * We can't use ereport, and libport can't use the logging from libcommon
+ * due to library order, so the options for logging are limited.
+ */
+ Assert(elevel == 0 || elevel == 1);
+#endif
+
if (attr == INVALID_FILE_ATTRIBUTES)
{
_dosmaperr(GetLastError());
+
+ if (errno == ENOENT && missing_ok)
+ return false;
+
+ if (elevel != 0)
+ {
+#ifndef FRONTEND
+ ereport(elevel,
+ (errcode_for_file_access(),
+ errmsg("could not get attributes for file \"%s\": %m",
+ path)));
+#else
+ int save_errno = errno;
+ fprintf(stderr, _("could not get attributes for file \"%s\": %s\n"),
+ path, strerror(save_errno));
+ exit(1);
+#endif
+ }
return false;
}
+ errno = 0;
return ((attr & FILE_ATTRIBUTE_REPARSE_POINT) == FILE_ATTRIBUTE_REPARSE_POINT);
}
#endif /* defined(WIN32) && !defined(__CYGWIN__) */
--
2.35.1
