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

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.

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

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

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.

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

With Regards,
Amit Kapila.