standby clone: fix data directory permissions handling for Pg11 and later

Previously, repmgr would forcibly change the permissions on a data
directory to 0700. However from PostgreSQL 11, 0750 is also valid,
so that value should not be changed.

HISTORY

@@ -2,6 +2,8 @@
         config: fix parsing of "replication_type"; GitHub #672 (Ian)
         standby clone: handle missing "postgresql.auto.conf" (Ian)
         standby clone: add option --recovery-min-apply-delay (Ian)
+        standby clone: fix data directory permissions handling for
+          PostgreSQL 11 and later (Ian)
         repmgrd: prevent termination when local node not available and
           standby_disconnect_on_failover; GitHub #675 (Ian)
         repmgrd: ensure reconnect_interval" is correctly handled;

dirutil.c

@@ -109,9 +109,56 @@ create_dir(const char *path)
 
 
 bool
-set_dir_permissions(const char *path)
+set_dir_permissions(const char *path, int server_version_num)
 {
-	return (chmod(path, 0700) != 0) ? false : true;
+	struct stat stat_buf;
+	bool no_group_access =
+		(server_version_num != UNKNOWN_SERVER_VERSION_NUM) &&
+		(server_version_num < 110000);
+	/*
+	 * At this point the path should exist, so this check is very
+	 * much just-in-case.
+	 */
+	if (stat(path, &stat_buf) != 0)
+	{
+		if (errno == ENOENT)
+		{
+			log_warning(_("directory \"%s\" does not exist"), path);
+		}
+		else
+		{
+			log_warning(_("could not read permissions of directory \"%s\""),
+						path);
+			log_detail("%s", strerror(errno));
+		}
+
+		return false;
+	}
+
+	/*
+	 * If mode is not 0700 or 0750, attempt to change.
+	 */
+	if ((no_group_access == true  && (stat_buf.st_mode & (S_IRWXG | S_IRWXO)))
+	 || (no_group_access == false && (stat_buf.st_mode & (S_IWGRP | S_IRWXO))))
+	{
+		/*
+		 * Currently we default to 0700.
+		 * There is no facility to override this directly,
+		 * but the user can manually create the directory with
+		 * the desired permissions.
+		 */
+
+		if (chmod(path, 0700) != 0) {
+			log_error(_("unable to change permissions of directory \"%s\""), path);
+			log_detail("%s", strerror(errno));
+			return false;
+		}
+
+		return true;
+	}
+
+	/* Leave as-is */
+	return true;
 }
 
 
@@ -303,7 +350,7 @@ create_pg_dir(const char *path, bool force)
 	switch (check_dir(path))
 	{
 		case DIR_NOENT:
-			/* directory does not exist, attempt to create it */
+			/* Directory does not exist, attempt to create it. */
 			log_info(_("creating directory \"%s\"..."), path);
 
 			if (!create_dir(path))
@@ -314,14 +361,23 @@ create_pg_dir(const char *path, bool force)
 			}
 			break;
 		case DIR_EMPTY:
-			/* exists but empty, fix permissions and use it */
+			/*
+			 * Directory exists but empty, fix permissions and use it.
+			 *
+			 * Note that at this point the caller might not know the server
+			 * version number, so in this case "set_dir_permissions()" will
+			 * accept 0750 as a valid setting. As this is invalid in Pg10 and
+			 * earlier,  the caller should call "set_dir_permissions()" again
+			 * when it has the number.
+			 *
+			 * We need to do the permissions check here in any case to catch
+			 * fatal permissions early.
+			 */
 			log_info(_("checking and correcting permissions on existing directory \"%s\""),
 					 path);
 
-			if (!set_dir_permissions(path))
+			if (!set_dir_permissions(path, UNKNOWN_SERVER_VERSION_NUM))
 			{
-				log_error(_("unable to change permissions of directory \"%s\""), path);
-				log_detail("%s", strerror(errno));
 				return false;
 			}
 			break;

dirutil.h

@@ -35,7 +35,7 @@ typedef enum
 } PgDirState;
 
 extern int	mkdir_p(char *path, mode_t omode);
-extern bool set_dir_permissions(const char *path);
+extern bool set_dir_permissions(const char *path, int server_version_num);
 
 extern DataDirState	check_dir(const char *path);
 extern bool create_dir(const char *path);

doc/appendix-release-notes.xml

@@ -46,12 +46,14 @@
 
       <title>Bug fixes</title>
       <para>
-        <itemizedlist>
+
+         <itemizedlist>
           <listitem>
             <para>
-              Fix parsing of <option>replication_type</option>.  GitHub #672.
+              Configuration: fix parsing of <option>replication_type</option>.  GitHub #672.
             </para>
           </listitem>
+
           <listitem>
             <para>
               <link linkend="repmgr-standby-clone">repmgr standby clone</link>:
@@ -60,6 +62,14 @@
             </para>
           </listitem>
 
+          <listitem>
+            <para>
+              <link linkend="repmgr-standby-clone">repmgr standby clone</link>:
+              in PostgreSQL 11 and later, an existing data directory's permissions
+              will not be changed to <option>0700</option> if they are already set to
+              <option>0750</option>.
+            </para>
+          </listitem>
 
           <listitem>
             <para>

repmgr-action-standby.c

@@ -838,6 +838,16 @@ do_standby_clone(void)
 			break;
 	}
 
+	/*
+	 * Do a final check on the data directory permissions - if the user
+	 * is cloning into an existing directory set to 0750, and the server
+	 * is Pg10 or earlier, Pg will refuse to start. We might not have
+	 * known the server version when creating the data directory
+	 * (mainly if cloning from Barman with no upstream connection), hence
+	 * the additional check here.
+	 */
+	set_dir_permissions(local_data_directory, source_server_version_num);
+
 	/*
 	 * TODO: It might be nice to provide an option to have repmgr start the
 	 * PostgreSQL server automatically