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

Hi,

2013-01-09 10:08 keltezéssel, Amit kapila írta:
> On Sunday, January 06, 2013 10:26 AM Boszormenyi Zoltan wrote:
> On Saturday, January 05, 2013 12:35 PM Boszormenyi Zoltan wrote:
> 2013-01-05 05:58 keltezéssel, Amit kapila írta:
>> On Friday, January 04, 2013 10:57 PM Boszormenyi Zoltan wrote:
>>> Hi,
>>> I am reviewing your patch.
>> Thank you very much.
>>
> All comments are addressed as below:
>
>> One specific comment about the documentation part of the patch:
> >
>> ***************
>> *** 86,92 **** SET [ SESSION | LOCAL ] TIME ZONE { <replaceable class="PARAMETER">timezone</rep
>> <application>PL/pgSQL</application> exception block. This behavior
>> has been changed because it was deemed unintuitive.
>> </para>
>> ! </note>
>> </refsect1>
>>
>> <refsect1>
>> --- 95,101 ----
>> <application>PL/pgSQL</application> exception block. This behavior
>> has been changed because it was deemed unintuitive.
>> </para>
>> ! </note>
>> </refsect1>
>>
>> <refsect1>
>> ***************
> Fixed the white space comment.
>
>> # This includes the default configuration directory included to support
>> # SET PERSISTENT statement.
>> #
>> # WARNNING: Any configuration parameter values specified after this line
>> # will override the values set by SET PERSISTENT statement.
>> include_dir = 'config_dir'
>>
>> Note the typo in the above message: WARNNING, there are too many Ns.
> Fixed.
>
>> Can't we add a new LWLock and use it as a critical section instead
>> of waiting for 10 seconds?
> Added LWLock and removed sleep logic. After adding the LWLock the lock file name can be changed
> as temp file. but presently the patch contains the name as lock file only. please provide the
> suggestions.

Current state of affairs:

a.) The whole operation is protected by the LWLock so only one backend
can do this any time. Clean operation is ensured.

b.) The code creates the lock file and fails if it the file exists. This protects
against nasties done externally. The current logic to bail out with an error
is OK, I can know that there is a stupid intruder on the system. But then
they can also remove the original .auto.conf file too and anything else and
I lost anyway.

c.) In reaper(), the code cleans up the lock file and since there can
be only one lock file, no need to search for temp files, a straightforward
unlink() call does its job.

This may be changed in two ways to make it more comfortable:

1. Simply unlink() and retry with creat() once.

Comments:
unlink() may fail under Windows if someone keeps this file open.
Then you can either give up with ereport(ERROR) just like now.

POSIX systems simply remove the file <-> inode mapping so unlink()
doesn't fail in this case.

2. Create the tempfile. There will be one less error case, the file creation
may only fail in case you are out of disk space.

Creating the tempfile is tempting. But then reaper() will need a little
more code to remove all the tempfiles.

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.

>> I also tried to trigger one of the errors by creating the lock file manually.
>> You need an extra space between the "... retry" and "or ...":
> Fixed.
>
>> Also, since the SET PERSISTENT patch seems to create a single lock file,
>> sizeof(string) (which includes the 0 byte at the end of the string, so it
>> matches the whole filename) can be used instead of strlen(string) or
>> sizeof(string)-1 that match the filename as a prefix only.
>> #define PGAUTOCONFLOCK "postgresql.auto.conf.lock"
> + /* skip lock files used in postgresql.auto.conf edit */
> + if (strncmp(de->d_name,
> + "postgresql.auto.conf.lock",
> + strlen("postgresql.auto.conf.lock")) == 0)
>
> Added a macro and changed the check accordingly.

If the current logic stays or the modification in 1) will be chosen,
the comment needs tweaking:

+ /* skip lock files used in postgresql.auto.conf edit */

"skip the lock file ..."

+ if (strncmp(de->d_name,
+ PGAUTOCONFLOCK,
+ sizeof(PGAUTOCONFLOCK)) == 0)
+ continue;
+

If the 2nd suggestion is chosen, sizeof()-1 or strlen() must be uses again
to exclude all the possible tempfiles.

Best regards,
Zoltán Böszörményi

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