On Mon, Jan 20, 2014 at 2:12 PM, Amit Kapila <amit.kapil...@gmail.com> wrote:
> On Sat, Jan 18, 2014 at 7:59 PM, Michael Paquier
> <michael.paqu...@gmail.com> wrote:
>> Hi all,
>>
>> After going through commit 65d6e4c (introducing ALTER SYSTEM SET), I
>> noticed a couple of typo mistakes as well as (I think) a weird way of
>> using the temporary auto-configuration name postgresql.auto.conf.temp
>> in two different places, resulting in the patch attached.
>
> .tmp suffix is used at couple other places in code as well.
> snapmgr.c
> XactExportFilePath(pathtmp, topXid, list_length(exportedSnapshots), ".tmp");
> receivelog.c
> snprintf(tmppath, MAXPGPATH, "%s.tmp", path);
>
> Although similar suffix is used at other places, but still I think it might be
> better to define for current case as here prefix (postgresql.auto.conf) is 
> also
> fixed and chances of getting it changed are less. However even if we don't
> do, it might be okay as we are using such suffixes at other places as well.
In the case of snapmgr.c and receivelog.c, the operations are kept
local, so it does not matter much if this way of naming is done as all
the operations for a temporary file are kept within the same, isolated
function. However, the case of postgresql.auto.conf.temp is different:
this temporary file name is used as well for a check in basebackup.c,
so I imagine that it would be better to centralize this information in
a single place. Now this name is only used in two places, but who
knows if some additional checks here are there won't be needed for
this temp file...
postgresql.auto.conf.temp is also located at the root of PGDATA, so it
might be surprising for the user to find it even if there are few
chances that it can happen.

>> It might be an overkill to use a dedicated variable for the temporary
>> autoconfiguration file (here PG_AUTOCONF_FILENAME_TEMP), but I found
>> kind of weird the way things are currently done on master branch. IMO,
>> it would reduce bug possibilities to have everything managed with a
>> single variable.
>>
>> Typos at least should be fixed :)
>
> - appendStringInfoString(&buf, "# Do not edit this file manually! \n");
> - appendStringInfoString(&buf, "# It will be overwritten by ALTER
> SYSTEM command. \n");
> + appendStringInfoString(&buf, "# Do not edit this file manually!\n");
> + appendStringInfoString(&buf, "# It will be overwritten by ALTER
> SYSTEM command.\n");
>
> Same change in initdb.c is missing.
Thanks, I missed it.
-- 
Michael
commit a8cf33deb3c46c4f56f1e617bbd3142cbbd66f1b
Author: Michael Paquier <mich...@otacoo.com>
Date:   Tue Jan 21 21:44:30 2014 +0900

    Fix typos and temporary file management in ALTER SYSTEM
    
    Using a temporary file uniquely defined in a single place reduces the
    occurence of bugs related to it. Some typos are fixed at the same time.

diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index fc35f5b..a31bcdf 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -834,11 +834,10 @@ sendDir(char *path, int basepathlen, bool sizeonly, List *tablespaces)
 
 		/* skip auto conf temporary file */
 		if (strncmp(de->d_name,
-					PG_AUTOCONF_FILENAME ".temp",
-					sizeof(PG_AUTOCONF_FILENAME) + 4) == 0)
+					PG_AUTOCONF_FILENAME_TEMP,
+					sizeof(PG_AUTOCONF_FILENAME_TEMP)) == 0)
 			continue;
 
-
 		/*
 		 * If there's a backup_label file, it belongs to a backup started by
 		 * the user with pg_start_backup(). It is *not* correct for this
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 1217098..7da52bf 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -6457,9 +6457,9 @@ flatten_set_variable_args(const char *name, List *args)
 }
 
 /*
- * Writes updated configuration parameter values into
- * postgresql.auto.conf.temp file. It traverses the list of parameters
- * and quote the string values before writing them to temporaray file.
+ * Write updated configuration parameter values into
+ * file PG_AUTOCONF_FILENAME_TEMP. It traverses the list of parameters
+ * and quotes the string values before writing them to temporary file.
  */
 static void
 write_auto_conf_file(int fd, const char *filename, ConfigVariable **head_p)
@@ -6468,8 +6468,8 @@ write_auto_conf_file(int fd, const char *filename, ConfigVariable **head_p)
 	StringInfoData buf;
 
 	initStringInfo(&buf);
-	appendStringInfoString(&buf, "# Do not edit this file manually! \n");
-	appendStringInfoString(&buf, "# It will be overwritten by ALTER SYSTEM command. \n");
+	appendStringInfoString(&buf, "# Do not edit this file manually!\n");
+	appendStringInfoString(&buf, "# It will be overwritten by ALTER SYSTEM command.\n");
 
 	/*
 	 * write the file header message before contents, so that if there is no
@@ -6596,7 +6596,7 @@ replace_auto_config_value(ConfigVariable **head_p, ConfigVariable **tail_p,
  *
  * The configuration parameters are written to a temporary
  * file then renamed to the final name. The template for the
- * temporary file is postgresql.auto.conf.temp.
+ * temporary file is PG_AUTOCONF_FILENAME_TEMP.
  *
  * An LWLock is used to serialize writing to the same file.
  *
@@ -6662,16 +6662,16 @@ AlterSystemSetConfigFile(AlterSystemStmt * altersysstmt)
 		ereport(ERROR,
 				(errmsg("invalid value for parameter \"%s\": \"%s\"", name, value)));
 
-
 	/*
 	 * Use data directory as reference path for postgresql.auto.conf and it's
-	 * corresponding temp file
+	 * corresponding temp file.
 	 */
-	join_path_components(AutoConfFileName, data_directory, PG_AUTOCONF_FILENAME);
+	join_path_components(AutoConfFileName, data_directory,
+						 PG_AUTOCONF_FILENAME);
 	canonicalize_path(AutoConfFileName);
-	snprintf(AutoConfTmpFileName, sizeof(AutoConfTmpFileName), "%s.%s",
-			 AutoConfFileName,
-			 "temp");
+	join_path_components(AutoConfTmpFileName, data_directory,
+						 PG_AUTOCONF_FILENAME_TEMP);
+	canonicalize_path(AutoConfTmpFileName);
 
 	/*
 	 * one backend is allowed to operate on postgresql.auto.conf file, to
@@ -6713,7 +6713,7 @@ AlterSystemSetConfigFile(AlterSystemStmt * altersysstmt)
 		 */
 		replace_auto_config_value(&head, &tail, AutoConfFileName, name, value);
 
-		/* Write and sync the New contents to postgresql.auto.conf.temp file */
+		/* Write and sync the New contents to file PG_AUTOCONF_FILENAME_TEMP */
 		write_auto_conf_file(Tmpfd, AutoConfTmpFileName, &head);
 
 		close(Tmpfd);
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 7e934b7..6b5302f 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -1300,8 +1300,8 @@ setup_config(void)
 	 * file will override the value of parameters that exists before parse of
 	 * this file.
 	 */
-	autoconflines[0] = pg_strdup("# Do not edit this file manually! \n");
-	autoconflines[1] = pg_strdup("# It will be overwritten by the ALTER SYSTEM command. \n");
+	autoconflines[0] = pg_strdup("# Do not edit this file manually!\n");
+	autoconflines[1] = pg_strdup("# It will be overwritten by the ALTER SYSTEM command.\n");
 	autoconflines[2] = NULL;
 
 	sprintf(path, "%s/%s", pg_data, PG_AUTOCONF_FILENAME);
diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h
index 20c5ff0..a4c2e3f 100644
--- a/src/include/pg_config_manual.h
+++ b/src/include/pg_config_manual.h
@@ -304,6 +304,12 @@
 /*
  * Automatic configuration file name for ALTER SYSTEM.
  * This file will be used to store values of configuration parameters
- * set by ALTER SYSTEM command
+ * set by ALTER SYSTEM command.
  */
 #define PG_AUTOCONF_FILENAME		"postgresql.auto.conf"
+
+/*
+ * This file will be used to store values in a temporary file
+ * when modifications occur with ALTER SYSTEM command.
+ */
+#define PG_AUTOCONF_FILENAME_TEMP	"postgresql.auto.conf.temp"
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to