On Friday, January 11, 2013 10:03 PM Boszormenyi Zoltan wrote:
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:
>>> 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
> 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.
> 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.
In response to
pgsql-hackers by date
|Next:||From: Hitoshi Harada||Date: 2013-01-12 06:28:15|
|Subject: Validation in to_date()|
|Previous:||From: Tory M Blue||Date: 2013-01-12 05:49:21|
|Subject: Wide area replication postgres 9.1.6 slon 2.1.2 large table failure.|