Re: Proposal for Allow postgresql.conf values to be changed via SQL [review]

2013-01-13 05:49 keltezéssel, Amit kapila írta:
> On Sunday, January 13, 2013 12:41 AM Tom Lane wrote:
> Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
>>> No, I mean the reaper(SIGNAL_ARGS) function in
>>> src/backend/postmaster/postmaster.c where your patch has this:
>>> *** a/src/backend/postmaster/postmaster.c
>>> --- b/src/backend/postmaster/postmaster.c
>>> ***************
>>> *** 2552,2557 **** reaper(SIGNAL_ARGS)
>> I have not been following this thread, but I happened to see this bit,
>> and I have to say that I am utterly dismayed that anything like this is
>> even on the table. This screams overdesign. We do not need a global
>> lock file, much less postmaster-side cleanup. All that's needed is a
>> suitable convention about temp file names that can be written and then
>> atomically renamed into place. If we're unlucky enough to crash before
>> a temp file can be renamed into place, it'll just sit there harmlessly.
> I can think of below ways to generate tmp file
> 1. Generate session specific tmp file name during operation. This will be similar to what is
> currently being done in OpenTemporaryFileinTablespace() to generate a tempfilename.
> 2. Generate global tmp file name. For this we need to maintain global counter.
> 3. Always generate temp file with same name "postgresql.auto.conf.tmp", as at a time only one
> session is allowed to operate.

What's wrong with mkstemp("config_dir/postgresql.auto.conf.XXXXXX")
that returns a file descriptor for an already created file with a unique name?

Note that the working directory of PostgreSQL is $PGDATA so "config_dir"
and files under that can be accessed with a relative path.

But a cleanup somewhere is needed to remove the stale temp files.
I think it's enough at postmaster startup. A machine that's crashing
so often that the presence of the stale temp files causes out of disk
errors would surely be noticed much earlier.

>
> In any approach, there will be chance that temp files will remain if server crashes during this command execution
> which can lead to collision of temp file name next time user tries to use SET persistent command.

mkstemp() replaces the "XXXXXX" suffix with a unique hexadecimal suffix
so there will be no collision.

> An appropriate error will be raised and user manually needs to delete that file.
>
>
>
>>>>> I just noticed that you are using "%m" in format strings twice. man 3 printf says:
>>>>> m (Glibc extension.) Print output of strerror(errno). No argument is required.
>>>>> This doesn't work anywhere else, only on GLIBC systems, it means Linux.
>>>>> I also like the brevity of this extension but PostgreSQL is a portable system.
>>>>> You should use %s and strerror(errno) as argument explicitly.
>>>> %m is used at other places in code as well.
>> As far as that goes, %m is appropriate in elog/ereport (which contain
>> special support for it), but not anywhere else.
> In the patch, it's used in ereport, so I assume it is safe and patch doesn't need any change for %m.

OK.

>
>
> With Regards,
> Amit Kapila.
>
>

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/