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.
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 :)
Regards,
--
Michael
commit 9886e788b27c8fce895cceacccc4c83ddd223bc2
Author: Michael Paquier <mich...@otacoo.com>
Date: Sat Jan 18 13:38:59 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/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
