2013-01-12 06:51 keltezéssel, Amit kapila írta:
> 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.
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:
*** 2552,2557 **** reaper(SIGNAL_ARGS)
--- 2552,2569 ----
+ /* Delete the postgresql.auto.conf.lock file if exists */
+ char LockFileName[MAXPGPATH];
+ strlcpy(LockFileName, ConfigFileName, sizeof(LockFileName));
+ join_path_components(LockFileName, 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.
>> 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.
Cybertec Schönig & Schönig GmbH
A-2700 Wiener Neustadt, Austria
In response to
pgsql-hackers by date
|Next:||From: Simon Riggs||Date: 2013-01-12 10:15:43|
|Subject: Re: Performance Improvement by reducing WAL for Update Operation|
|Previous:||From: Hitoshi Harada||Date: 2013-01-12 06:28:15|
|Subject: Validation in to_date()|