On 30.10.23 12:55, Aleksander Alekseev wrote:
The patch LGTM. However, postgresql:pg_resetwal test suite doesn't
pass on Windows according to cfbot. Seems to be a matter of picking a
more generic regular expression:
```
at C:/cirrus/src/bin/pg_resetwal/t/001_basic.pl line 54.
'pg_resetwal: error: could not change directory to
"foo": No such file or directory
doesn't match '(?^:error: could not read permissions of directory)'
```
Should we simply use something like:
```
qr/error: could not (read|change).* directory/
```
Hmm. I think maybe we should fix the behavior of
GetDataDirectoryCreatePerm() to be more consistent between Windows and
non-Windows. This is usually the first function a program uses on the
proposed data directory, so it's also responsible for reporting if the
data directory does not exist. But then on Windows, because the
function does nothing, those error scenarios end up on quite different
code paths, and I'm not sure if those are really checked that carefully.
I think we can make this more robust if we have
GetDataDirectoryCreatePerm() still run the stat() call on the proposed
data directory and report the error. See attached patch.From ff3a5ef09ea7f6f54262986e234ec919d53821ac Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Tue, 31 Oct 2023 07:42:17 -0400
Subject: [PATCH] More consistent behavior of GetDataDirectoryCreatePerm on
Windows
On Windows, GetDataDirectoryCreatePerm() just did nothing. The way
the code in some callers is structured, this is the first function
that tries to address the data directory. So it also ends up the
place that is responsible for reporting that a data directory does not
exist or similar. Therefore, on Windows, these scenarios end up on a
potentially completely different code paths.
To unify this, to make testing more consistent across platforms, have
GetDataDirectoryCreatePerm() run the stat() call on Windows as well,
even though it won't do anything with the result. That way, file
system errors are reporting to callers in the same way as on
non-Windows.
---
src/common/file_perm.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/src/common/file_perm.c b/src/common/file_perm.c
index 60f88d2caf1..81464f939ca 100644
--- a/src/common/file_perm.c
+++ b/src/common/file_perm.c
@@ -59,12 +59,12 @@ SetDataDirectoryCreatePerm(int dataDirMode)
* false is returned.
*
* Suppress when on Windows, because there may not be proper support for Unix-y
- * file permissions.
+ * file permissions. But we still run stat() on the directory so that callers
+ * get consistent behavior for example if the directory does not exist.
*/
bool
GetDataDirectoryCreatePerm(const char *dataDir)
{
-#if !defined(WIN32) && !defined(__CYGWIN__)
struct stat statBuf;
/*
@@ -75,16 +75,12 @@ GetDataDirectoryCreatePerm(const char *dataDir)
if (stat(dataDir, &statBuf) == -1)
return false;
+#if !defined(WIN32) && !defined(__CYGWIN__)
/* Set permissions */
SetDataDirectoryCreatePerm(statBuf.st_mode);
- return true;
-#else /* !defined(WIN32) &&
!defined(__CYGWIN__) */
- /*
- * On Windows, we don't have anything to do here since they don't have
- * Unix-y permissions.
- */
- return true;
#endif
+
+ return true;
}
--
2.42.0
