Fix incorrect ordering of operations in pg_resetwal and pg_rewind.

Commit c37b3d08c dropped its added GetDataDirectoryCreatePerm call into
the wrong place in pg_resetwal.c, namely after the chdir to DataDir.
That broke invocations using a relative path, as reported by Tushar Ahuja.
We could have left it where it was and changed the argument to be ".",
but that'd result in a rather confusing error message in event of a
failure, so re-ordering seems like a better solution.

Similarly reorder operations in pg_rewind.c.  The issue there is that
it doesn't seem like a good idea to do any actual operations before the
not-root check (on Unix) or the restricted token acquisition (on Windows).
I don't know that this is an actual bug, but I'm definitely not convinced
that it isn't, either.

Assorted other code review for c37b3d08c and da9b580d8: fix some
misspelled or otherwise badly worded comments, put the #include for
<sys/stat.h> where it actually belongs, etc.

Discussion: https://postgr.es/m/aeb9c3a7-3c3f-a57f-1a18-c8d4fcdc2a1f@enterprisedb.com

diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 441f18dcf56e6fd679269806da46f7281c58d85d..8dd51f176749c5d1b2adc9993b5a9a3571ee566c 100644 (file)
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -3552,8 +3552,8 @@ fsync_parent_path(const char *fname, int elevel)
 /*
  * Create a PostgreSQL data sub-directory
  *
- * The data directory itself, along with most other directories, are created at
- * initdb-time, but we do have some occations where we create directories from
+ * The data directory itself, and most of its sub-directories, are created at
+ * initdb time, but we do have some occasions when we create directories in
  * the backend (CREATE TABLESPACE, for example).  In those cases, we want to
  * make sure that those directories are created consistently.  Today, that means
  * making sure that the created directory has the correct permissions, which is
@@ -3562,8 +3562,8 @@ fsync_parent_path(const char *fname, int elevel)
  * Note that we also set the umask() based on what we understand the correct
  * permissions to be (see file_perm.c).
  *
- * For permissions other than the default mkdir() can be used directly, but be
- * sure to consider carefully such cases -- a directory with incorrect
+ * For permissions other than the default, mkdir() can be used directly, but
+ * be sure to consider carefully such cases -- a sub-directory with incorrect
  * permissions in a PostgreSQL data directory could cause backups and other
  * processes to fail.
  */

diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 36ffd874a404f3dc6b380ba04ac5654ac28038d7..f7d6617a138a441bc1ad48ff8166a2bdf17a56b4 100644 (file)
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -18,8 +18,6 @@
  */
 #include "postgres.h"
 
-#include <sys/stat.h>
-
 #include "common/file_perm.h"
 #include "libpq/libpq-be.h"
 #include "libpq/pqcomm.h"
@@ -63,7 +61,7 @@ struct Latch *MyLatch;
 char      *DataDir = NULL;
 
 /*
- * Mode of the data directory.  The default is 0700 but may it be changed in
+ * Mode of the data directory.  The default is 0700 but it may be changed in
  * checkDataDir() to 0750 if the data directory actually has that mode.
  */
 int                    data_directory_mode = PG_DIR_MODE_OWNER;

diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c
index ee28c338f88dde01543af4c5ea9ecec8e47f65cf..8cff5356925647fa58e30cd5c4f60aa47ba2845d 100644 (file)
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -356,13 +356,6 @@ main(int argc, char *argv[])
 
        get_restricted_token(progname);
 
-       if (chdir(DataDir) < 0)
-       {
-               fprintf(stderr, _("%s: could not change directory to \"%s\": %s\n"),
-                               progname, DataDir, strerror(errno));
-               exit(1);
-       }
-
        /* Set mask based on PGDATA permissions */
        if (!GetDataDirectoryCreatePerm(DataDir))
        {
@@ -373,6 +366,13 @@ main(int argc, char *argv[])
 
        umask(pg_mode_mask);
 
+       if (chdir(DataDir) < 0)
+       {
+               fprintf(stderr, _("%s: could not change directory to \"%s\": %s\n"),
+                               progname, DataDir, strerror(errno));
+               exit(1);
+       }
+
        /* Check that data directory matches our server version */
        CheckDataVersion();
 

diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index cabcff5c13b2508cbde438258df910f09083a767..b0fd3f66ace41ffa1fbd4727128a2123a6bee2c7 100644 (file)
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -186,16 +186,6 @@ main(int argc, char **argv)
                exit(1);
        }
 
-       /* Set mask based on PGDATA permissions */
-       if (!GetDataDirectoryCreatePerm(datadir_target))
-       {
-               fprintf(stderr, _("%s: could not read permissions of directory \"%s\": %s\n"),
-                               progname, datadir_target, strerror(errno));
-               exit(1);
-       }
-
-       umask(pg_mode_mask);
-
        /*
         * Don't allow pg_rewind to be run as root, to avoid overwriting the
         * ownership of files in the data directory. We need only check for root
@@ -214,6 +204,16 @@ main(int argc, char **argv)
 
        get_restricted_token(progname);
 
+       /* Set mask based on PGDATA permissions */
+       if (!GetDataDirectoryCreatePerm(datadir_target))
+       {
+               fprintf(stderr, _("%s: could not read permissions of directory \"%s\": %s\n"),
+                               progname, datadir_target, strerror(errno));
+               exit(1);
+       }
+
+       umask(pg_mode_mask);
+
        /* Connect to remote server */
        if (connstr_source)
                libpqConnect(connstr_source);

diff --git a/src/common/file_perm.c b/src/common/file_perm.c
index f2c8f84609390472af412fbd063cda470e09eefb..9fd11df2886fe3efc2d933225355191c69344d01 100644 (file)
--- a/src/common/file_perm.c
+++ b/src/common/file_perm.c
@@ -10,9 +10,8 @@
  *
  *-------------------------------------------------------------------------
  */
-#include <sys/stat.h>
-
 #include "c.h"
+
 #include "common/file_perm.h"
 
 /* Modes for creating directories and files in the data directory */

diff --git a/src/include/common/file_perm.h b/src/include/common/file_perm.h
index 3090f789317be42c04ccb1f6b3ec8844970e4b5a..cfa0546385a73c4977d01fca06f29a82e9fa0596 100644 (file)
--- a/src/include/common/file_perm.h
+++ b/src/include/common/file_perm.h
@@ -1,6 +1,6 @@
 /*-------------------------------------------------------------------------
  *
- * File and directory permission constants
+ * File and directory permission definitions
  *
  *
  * Portions Copyright (c) 1996-2018, PostgreSQL Global Development Group
@@ -13,6 +13,8 @@
 #ifndef FILE_PERM_H
 #define FILE_PERM_H
 
+#include <sys/stat.h>
+
 /*
  * Mode mask for data directory permissions that only allows the owner to
  * read/write directories and files.