Here's a better idea, now that I'm emboldened by having working CI for
Windows frankenbuilds, and since I broke some stuff in this area on
MSYS[1], which caused me to look more closely at this area.
Why don't we just nuke pgwin32_is_junction() from orbit, and teach
Windows how to lstat()? We're already defining our own replacement
stat() used in both MSVC and MSYS builds along with our own junction
point-based symlink() and readlink() functions, and lstat() was
already suggested in a comment in win32stat.c.
There's one curious change in the draft patch attached: you can't
unlink() a junction point, you have to rmdir() it. Previously, things
that traverse directories without ever calling pgwin32_is_junction()
would see junction points as S_ISDIR() and call rmdir(), which was OK,
but now they see S_ISLNK() and call unlink(). So I taught unlink() to
try both things. Which is kinda weird, and not beautiful, especially
when combined with the existing looping weirdness.
0001 is a copy of v2 of Melih Mutlu's CI patch[2] to show cfbot how to
test this on MSYS (alongside the normal MSVC result), but that's not
part of this submission.
[1]
https://www.postgresql.org/message-id/flat/b9ddf605-6b36-f90d-7c30-7b3e95c46276%40dunslane.net
[2]
https://www.postgresql.org/message-id/flat/CAGPVpCSKS9E0An4%3De7ZDnme%2By%3DWOcQFJYJegKO8kE9%3Dgh8NJKQ%40mail.gmail.com
From ec732571a1fa88fefcf9834987114716eb9c5998 Mon Sep 17 00:00:00 2001
From: Melih Mutlu <memu...@microsoft.com>
Date: Mon, 21 Feb 2022 14:46:05 +0300
Subject: [PATCH 1/3] Added Windows with MinGW environment in Cirrus CI
---
.cirrus.yml | 79 +++++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 65 insertions(+), 14 deletions(-)
diff --git a/.cirrus.yml b/.cirrus.yml
index f23d6cae55..3d8c4cd813 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -23,7 +23,6 @@ env:
CHECKFLAGS: -Otarget
PROVE_FLAGS: --timer
PGCTLTIMEOUT: 120 # avoids spurious failures during parallel tests
- TEMP_CONFIG: ${CIRRUS_WORKING_DIR}/src/tools/ci/pg_ci_base.conf
PG_TEST_EXTRA: kerberos ldap ssl
@@ -338,13 +337,30 @@ task:
cores_script: src/tools/ci/cores_backtrace.sh macos "${HOME}/cores"
+WINDOWS_ENVIRONMENT_BASE: &WINDOWS_ENVIRONMENT_BASE
+ env:
+ # Half the allowed per-user CPU cores
+ CPUS: 4
+ # The default working dir is in a directory msbuild complains about
+ CIRRUS_WORKING_DIR: "c:/cirrus"
+ TEMP_CONFIG: ${CIRRUS_WORKING_DIR}/src/tools/ci/pg_ci_base.conf
+
+ # Avoids port conflicts between concurrent tap test runs
+ PG_TEST_USE_UNIX_SOCKETS: 1
+
+ only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*windows.*'
+
+ sysinfo_script: |
+ chcp
+ systeminfo
+ powershell -Command get-psdrive -psprovider filesystem
+ set
+
task:
+ << : *WINDOWS_ENVIRONMENT_BASE
name: Windows - Server 2019, VS 2019
env:
- # Half the allowed per-user CPU cores
- CPUS: 4
-
# Our windows infrastructure doesn't have test concurrency above the level
# of a single vcregress test target. Due to that, it's useful to run prove
# with multiple jobs. For the other tasks it isn't, because two sources
@@ -354,15 +370,11 @@ task:
# likely can be improved upon further.
PROVE_FLAGS: -j10 --timer
- # The default cirrus working dir is in a directory msbuild complains about
- CIRRUS_WORKING_DIR: "c:/cirrus"
# Avoid re-installing over and over
NO_TEMP_INSTALL: 1
# git's tar doesn't deal with drive letters, see
# https://postgr.es/m/b6782dc3-a7b0-ed56-175f-f8f54cb08d67%40dunslane.net
TAR: "c:/windows/system32/tar.exe"
- # Avoids port conflicts between concurrent tap test runs
- PG_TEST_USE_UNIX_SOCKETS: 1
PG_REGRESS_SOCK_DIR: "c:/cirrus/"
# -m enables parallelism
# verbosity:minimal + Summary reduce verbosity, while keeping a summary of
@@ -393,12 +405,6 @@ task:
cpu: $CPUS
memory: 4G
- sysinfo_script: |
- chcp
- systeminfo
- powershell -Command get-psdrive -psprovider filesystem
- set
-
setup_additional_packages_script: |
REM choco install -y --no-progress ...
@@ -456,6 +462,51 @@ task:
path: "crashlog-*.txt"
type: text/plain
+task:
+ << : *WINDOWS_ENVIRONMENT_BASE
+ name: Windows - Server 2019, MinGW64
+ windows_container:
+ image: $CONTAINER_REPO/windows_ci_mingw64:latest
+ cpu: $CPUS
+ memory: 4G
+ env:
+ CCACHE_DIR: C:/msys64/ccache
+ BUILD_DIR: "%CIRRUS_WORKING_DIR%/build"
+
+ ccache_cache:
+ folder: ${CCACHE_DIR}
+
+ mingw_info_script:
+ - C:\msys64\usr\bin\bash.exe -lc "where gcc"
+ - C:\msys64\usr\bin\bash.exe -lc "gcc --version"
+ - C:\msys64\usr\bin\bash.exe -lc "where perl"
+ - C:\msys64\usr\bin\bash.exe -lc "perl --version"
+
+ configure_script:
+ - C:\msys64\usr\bin\bash.exe -lc "mkdir %BUILD_DIR% &&
+ cd %BUILD_DIR% &&
+ %CIRRUS_WORKING_DIR%/configure
+ --host=x86_64-w64-mingw32
+ --enable-cassert
+ --enable-tap-tests
+ --with-icu
+ --with-libxml
+ --with-libxslt
+ --with-lz4
+ --enable-debug
+ CC='ccache gcc'
+ CXX='ccache g++'"
+
+ build_script:
+ C:\msys64\usr\bin\bash.exe -lc "cd %BUILD_DIR% && make -s world-bin -j${CPUS}"
+
+ upload_caches: ccache
+
+ tests_script:
+ - set "NoDefaultCurrentDirectoryInExePath=0"
+ - C:\msys64\usr\bin\bash.exe -lc "cd %BUILD_DIR% && make -s ${CHECK} ${CHECKFLAGS} -j${CPUS} TMPDIR=%BUILD_DIR%/tmp_install"
+
+ on_failure: *on_failure
task:
name: CompilerWarnings
--
2.36.1
From 2390efc8ffebcb975c0c96b63479a1f22bf8e422 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Thu, 28 Jul 2022 15:19:38 +1200
Subject: [PATCH 2/3] Provide lstat() for Windows.
Junction points will be reported with S_ISLNK(x.st_mode), like in POSIX.
stat() will follow symlinks, like in POSIX (but only one level before it
gives up, unlike in POSIX).
Discussion: https://postgr.es/m/CA%2BhUKGLfOOeyZpm5ByVcAt7x5Pn-%3DxGRNCvgiUPVVzjFLtnY0w%40mail.gmail.com
---
src/include/port/win32_port.h | 18 +++++++-
src/port/win32stat.c | 79 +++++++++++++++++++++++++++++++++--
2 files changed, 92 insertions(+), 5 deletions(-)
diff --git a/src/include/port/win32_port.h b/src/include/port/win32_port.h
index 4de5bf3bf6..b8cf2e1480 100644
--- a/src/include/port/win32_port.h
+++ b/src/include/port/win32_port.h
@@ -286,10 +286,11 @@ struct stat /* This should match struct __stat64 */
extern int _pgfstat64(int fileno, struct stat *buf);
extern int _pgstat64(const char *name, struct stat *buf);
+extern int _pglstat64(const char *name, struct stat *buf);
#define fstat(fileno, sb) _pgfstat64(fileno, sb)
#define stat(path, sb) _pgstat64(path, sb)
-#define lstat(path, sb) _pgstat64(path, sb)
+#define lstat(path, sb) _pglstat64(path, sb)
/* These macros are not provided by older MinGW, nor by MSVC */
#ifndef S_IRUSR
@@ -335,6 +336,21 @@ extern int _pgstat64(const char *name, struct stat *buf);
#define S_ISREG(m) (((m) & S_IFMT) == S_IFREG)
#endif
+/*
+ * In order for lstat() to be able to report junction points as symlinks, we
+ * need to hijack a bit in st_mode, since neither MSVC nor MinGW provides
+ * S_ISLNK and there aren't any spare bits. We'll steal the one for character
+ * devices, because we don't otherwise make use of those.
+ */
+#ifdef S_ISLNK
+#error "S_ISLNK is already defined"
+#endif
+#ifdef S_IFLNK
+#error "S_IFLNK is already defined"
+#endif
+#define S_IFLNK S_IFCHR
+#define S_ISLNK(m) (((m) & S_IFLNK) == S_IFLNK)
+
/*
* Supplement to <fcntl.h>.
* This is the same value as _O_NOINHERIT in the MS header file. This is
diff --git a/src/port/win32stat.c b/src/port/win32stat.c
index e03ed5f35c..686c510fed 100644
--- a/src/port/win32stat.c
+++ b/src/port/win32stat.c
@@ -15,7 +15,11 @@
#ifdef WIN32
+#define UMDF_USING_NTSTATUS
+
#include "c.h"
+#include "port/win32ntdll.h"
+
#include <windows.h>
/*
@@ -107,12 +111,10 @@ fileinfo_to_stat(HANDLE hFile, struct stat *buf)
}
/*
- * Windows implementation of stat().
- *
- * This currently also implements lstat(), though perhaps that should change.
+ * Windows implementation of lstat().
*/
int
-_pgstat64(const char *name, struct stat *buf)
+_pglstat64(const char *name, struct stat *buf)
{
/*
* Our open wrapper will report STATUS_DELETE_PENDING as ENOENT. We
@@ -129,10 +131,79 @@ _pgstat64(const char *name, struct stat *buf)
ret = fileinfo_to_stat(hFile, buf);
+ /*
+ * Unfortunately it's not possible for fileinfo_to_stat() to see a
+ * FILE_ATTRIBUTE_REPARSE_POINT flag with just a handle, so at this point
+ * junction points appear as directories. Ask for the full attributes
+ * while we still have the handle open. Someone else can unlink the file
+ * while we have it open, but they can't create another file with the same
+ * name.
+ */
+ if (ret == 0 && S_ISDIR(buf->st_mode))
+ {
+ DWORD attr;
+
+ attr = GetFileAttributes(name);
+ if (attr == INVALID_FILE_ATTRIBUTES)
+ {
+ DWORD err;
+
+ /* If it's been unlinked since we opened it, report as ENOENT. */
+ err = GetLastError();
+ if (err == ERROR_ACCESS_DENIED &&
+ pg_RtlGetLastNtStatus() == STATUS_DELETE_PENDING)
+ errno = ENOENT;
+ else
+ _dosmaperr(err);
+
+ ret = -1;
+ }
+ else if (attr & FILE_ATTRIBUTE_REPARSE_POINT)
+ {
+ buf->st_mode &= ~S_IFDIR;
+ buf->st_mode |= S_IFLNK;
+ }
+ }
+
CloseHandle(hFile);
return ret;
}
+/*
+ * Windows implementation of stat().
+ */
+int
+_pgstat64(const char *name, struct stat *buf)
+{
+ int ret;
+
+ ret = _pglstat64(name, buf);
+
+ /* Do we need to follow a symlink (junction point)? */
+ if (ret == 0 && S_ISLNK(buf->st_mode))
+ {
+ char next[MAXPGPATH];
+
+ if (readlink(name, next, sizeof(next)) < 0)
+ return -1;
+
+ ret = _pglstat64(next, buf);
+ if (ret == 0 && S_ISLNK(buf->st_mode))
+ {
+ /*
+ * We're only prepared to go one hop, because we only expect to
+ * deal with the simple cases that we create. The error for too
+ * many symlinks is supposed to be ELOOP, but Windows hasn't got
+ * it.
+ */
+ errno = EIO;
+ return -1;
+ }
+ }
+
+ return ret;
+}
+
/*
* Windows implementation of fstat().
*/
--
2.36.1
From 2d44c134f62872889439db6910709af745901dd9 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Thu, 28 Jul 2022 15:25:10 +1200
Subject: [PATCH 3/3] Remove pgwin32_is_junction().
Now that lstat() reports junction points with S_IFLNK, there is no need
for sparate code paths for Windows in a few places. The coding around
pgwin32_is_junction() was a bit suspect anyway, as we never checked for
errors.
Discussion: https://postgr.es/m/CA%2BhUKGLfOOeyZpm5ByVcAt7x5Pn-%3DxGRNCvgiUPVVzjFLtnY0w%40mail.gmail.com
---
src/backend/commands/tablespace.c | 3 +--
src/backend/replication/basebackup.c | 12 +-----------
src/backend/storage/file/fd.c | 5 -----
src/backend/utils/adt/misc.c | 7 -------
src/bin/pg_checksums/pg_checksums.c | 4 ----
src/bin/pg_rewind/file_ops.c | 4 ----
src/common/file_utils.c | 21 --------------------
src/include/port.h | 1 -
src/include/port/win32_port.h | 1 -
src/port/dirmod.c | 29 +++++++++++++---------------
10 files changed, 15 insertions(+), 72 deletions(-)
diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
index cb7d46089a..b74f40aac6 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -827,8 +827,7 @@ destroy_tablespace_directories(Oid tablespaceoid, bool redo)
/*
* Try to remove the symlink. We must however deal with the possibility
* that it's a directory instead of a symlink --- this could happen during
- * WAL replay (see TablespaceCreateDbspace), and it is also the case on
- * Windows where junction points lstat() as directories.
+ * WAL replay (see TablespaceCreateDbspace).
*
* Note: in the redo case, we'll return true if this final step fails;
* there's no point in retrying it. Also, ENOENT should provoke no more
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index 637c0ce459..638de07035 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -1320,13 +1320,7 @@ sendDir(bbsink *sink, const char *path, int basepathlen, bool sizeonly,
}
/* Allow symbolic links in pg_tblspc only */
- if (strcmp(path, "./pg_tblspc") == 0 &&
-#ifndef WIN32
- S_ISLNK(statbuf.st_mode)
-#else
- pgwin32_is_junction(pathbuf)
-#endif
- )
+ if (strcmp(path, "./pg_tblspc") == 0 && S_ISLNK(statbuf.st_mode))
{
#if defined(HAVE_READLINK) || defined(WIN32)
char linkpath[MAXPGPATH];
@@ -1809,11 +1803,7 @@ static void
convert_link_to_directory(const char *pathbuf, struct stat *statbuf)
{
/* If symlink, write it as a directory anyway */
-#ifndef WIN32
if (S_ISLNK(statbuf->st_mode))
-#else
- if (pgwin32_is_junction(pathbuf))
-#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 f904f60c08..566ebe26a0 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -3377,7 +3377,6 @@ SyncDataDirectory(void)
*/
xlog_is_symlink = false;
-#ifndef WIN32
{
struct stat st;
@@ -3389,10 +3388,6 @@ SyncDataDirectory(void)
else if (S_ISLNK(st.st_mode))
xlog_is_symlink = true;
}
-#else
- if (pgwin32_is_junction("pg_wal"))
- xlog_is_symlink = true;
-#endif
#ifdef HAVE_SYNCFS
if (recovery_init_sync_method == RECOVERY_INIT_SYNC_METHOD_SYNCFS)
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index 89690be2ed..412039ea6b 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -283,9 +283,7 @@ pg_tablespace_location(PG_FUNCTION_ARGS)
char sourcepath[MAXPGPATH];
char targetpath[MAXPGPATH];
int rllen;
-#ifndef WIN32
struct stat st;
-#endif
/*
* It's useful to apply this function to pg_class.reltablespace, wherein
@@ -316,10 +314,6 @@ pg_tablespace_location(PG_FUNCTION_ARGS)
* created with allow_in_place_tablespaces enabled. If a directory is
* found, a relative path to the data directory is returned.
*/
-#ifdef WIN32
- if (!pgwin32_is_junction(sourcepath))
- PG_RETURN_TEXT_P(cstring_to_text(sourcepath));
-#else
if (lstat(sourcepath, &st) < 0)
{
ereport(ERROR,
@@ -330,7 +324,6 @@ pg_tablespace_location(PG_FUNCTION_ARGS)
if (!S_ISLNK(st.st_mode))
PG_RETURN_TEXT_P(cstring_to_text(sourcepath));
-#endif
/*
* In presence of a link or a junction point, return the path pointing to.
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index dc20122c89..324ccf7783 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -384,11 +384,7 @@ scan_directory(const char *basedir, const char *subdir, bool sizeonly)
if (!sizeonly)
scan_file(fn, segmentno);
}
-#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))
-#endif
{
/*
* If going through the entries of pg_tblspc, we assume to operate
diff --git a/src/bin/pg_rewind/file_ops.c b/src/bin/pg_rewind/file_ops.c
index 6cb288f099..7d98283c79 100644
--- a/src/bin/pg_rewind/file_ops.c
+++ b/src/bin/pg_rewind/file_ops.c
@@ -431,11 +431,7 @@ recurse_dir(const char *datadir, const char *parentpath,
/* recurse to handle subdirectories */
recurse_dir(datadir, path, callback);
}
-#ifndef WIN32
else if (S_ISLNK(fst.st_mode))
-#else
- else if (pgwin32_is_junction(fullpath))
-#endif
{
#if defined(HAVE_READLINK) || defined(WIN32)
char link_target[MAXPGPATH];
diff --git a/src/common/file_utils.c b/src/common/file_utils.c
index 966b987d64..71c232d983 100644
--- a/src/common/file_utils.c
+++ b/src/common/file_utils.c
@@ -79,7 +79,6 @@ fsync_pgdata(const char *pg_data,
*/
xlog_is_symlink = false;
-#ifndef WIN32
{
struct stat st;
@@ -88,10 +87,6 @@ fsync_pgdata(const char *pg_data,
else if (S_ISLNK(st.st_mode))
xlog_is_symlink = true;
}
-#else
- if (pgwin32_is_junction(pg_wal))
- xlog_is_symlink = true;
-#endif
/*
* If possible, hint to the kernel that we're soon going to fsync the data
@@ -465,21 +460,5 @@ get_dirent_type(const char *path,
#endif
}
-#if defined(WIN32) && !defined(_MSC_VER)
-
- /*
- * If we're on native Windows (not Cygwin, which has its own POSIX
- * symlinks), but not using the MSVC compiler, then we're using a
- * readdir() emulation provided by the MinGW runtime that has no d_type.
- * Since the lstat() fallback code reports junction points as directories,
- * we need an extra system call to check if we should report them as
- * symlinks instead, following our convention.
- */
- if (result == PGFILETYPE_DIR &&
- !look_through_symlinks &&
- pgwin32_is_junction(path))
- result = PGFILETYPE_LNK;
-#endif
-
return result;
}
diff --git a/src/include/port.h b/src/include/port.h
index d39b04141f..4b978f61bd 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -284,7 +284,6 @@ 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);
#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 b8cf2e1480..72a0dd572e 100644
--- a/src/include/port/win32_port.h
+++ b/src/include/port/win32_port.h
@@ -238,7 +238,6 @@ 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);
#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..01f9d1362b 100644
--- a/src/port/dirmod.c
+++ b/src/port/dirmod.c
@@ -111,6 +111,19 @@ pgunlink(const char *path)
{
if (errno != EACCES)
return -1;
+
+ /*
+ * Also try with rmdir(), because that's how you remove junction
+ * points. Since we pretend junction points are symlinks, various code
+ * might try to call unlink(). We'll preserve unlink()'s errno though;
+ * if we got ENOENT then unlink() will presumably return the same next
+ * time around, ENOTEMPTY shouldn't be possible for a junction point,
+ * and EACCES is what unlink() returned.
+ */
+ if (rmdir(path) == 0)
+ break;
+ errno = EACCES;
+
if (++loops > 100) /* time out after 10 sec */
return -1;
pg_usleep(100000); /* us */
@@ -336,20 +349,4 @@ pgreadlink(const char *path, char *buf, size_t size)
return r;
}
-/*
- * Assumes the file exists, so will return false if it doesn't
- * (since a nonexistent file is not a junction)
- */
-bool
-pgwin32_is_junction(const char *path)
-{
- DWORD attr = GetFileAttributes(path);
-
- if (attr == INVALID_FILE_ATTRIBUTES)
- {
- _dosmaperr(GetLastError());
- return false;
- }
- return ((attr & FILE_ATTRIBUTE_REPARSE_POINT) == FILE_ATTRIBUTE_REPARSE_POINT);
-}
#endif /* defined(WIN32) && !defined(__CYGWIN__) */
--
2.36.1
