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/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
