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

On Wed, Jan 22, 2014 at 1:15 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Jan 21, 2014 at 7:47 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>>> 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.
>
> I don't think there's any real reason to defined
> PG_AUTOCONF_FILENAME_TEMP. pg_stat_statements just writes
> PGSS_DUMP_FILE ".tmp" and that hasn't been a problem that I know of.
> I do wonder why ALTER SYSTEM SET is spelling the suffix "temp" instead
> of "tmp".

+1 for changing '.temp' to '.tmp'.
I think its bit arguable whether to change it to define or not (different people
can have different opinion on this matter), but certainly changing it to .tmp
is an improvement, as we use .tmp at other places as well.

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