Re: Proposal for Allow postgresql.conf values to be changed via SQL [review] - Mailing list pgsql-hackers

2013-01-12 06:51 keltezéssel, Amit kapila írta:
> On Friday, January 11, 2013 10:03 PM Boszormenyi Zoltan wrote:
>> 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:
>>
>
>>>> 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.
> I think as this is an internal file, user is not supposed to play with file and incase he does so,
> then I think current implementation is okay, means during open (O_CREAT) it gives error and the message
> is also suggesting the same.

That's what I meant in b) above.

>
>
>
>> 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.
> By reaper you mean to say CATCH block?

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)
--- 2552,2569 ----                                continue;                        }

+                       /* Delete the postgresql.auto.conf.lock file if exists */
+                       {
+                               char LockFileName[MAXPGPATH];
+
+                               strlcpy(LockFileName, ConfigFileName, sizeof(LockFileName));
+                               get_parent_directory(LockFileName);
+                               join_path_components(LockFileName, LockFileName,
AutoConfigLockFilename);
+                               canonicalize_path(LockFileName);
+
+                               unlink(LockFileName);
+                       }
+                        /*                         * Startup succeeded, commence normal operations
   */ 


>
> In that case, I would prefer to keep the current implementation as it is.

I also said above the current logic is OK for me.
I just gave food for thought to provoke discussion from others.

>
> Actualy I was thinking of just changing the extension from current .lock to .tmp, so in that case
> the same problems can happen with this also.

Indeed.

>
>> 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.
>
> Thank you for feedback.

You're welcome.

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