Re: ALTER SYSTEM SET typos and fix for temporary file name management

On Sat, Jan 18, 2014 at 7:59 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)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.

> 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.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com