2013-01-13 05:49 keltezéssel, Amit kapila írta:
On Sunday, January 13, 2013 12:41 AM Tom Lane wrote:
Boszormenyi Zoltan <z...@cybertec.at> writes:
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)
I have not been following this thread, but I happened to see this bit,
and I have to say that I am utterly dismayed that anything like this is
even on the table. This screams overdesign. We do not need a global
lock file, much less postmaster-side cleanup. All that's needed is a
suitable convention about temp file names that can be written and then
atomically renamed into place. If we're unlucky enough to crash before
a temp file can be renamed into place, it'll just sit there harmlessly.
I can think of below ways to generate tmp file
1. Generate session specific tmp file name during operation. This will be
similar to what is
currently being done in OpenTemporaryFileinTablespace() to generate a
tempfilename.
2. Generate global tmp file name. For this we need to maintain global counter.
3. Always generate temp file with same name "postgresql.auto.conf.tmp", as at a
time only one
session is allowed to operate.
What's wrong with mkstemp("config_dir/postgresql.auto.conf.XXXXXX")
that returns a file descriptor for an already created file with a unique name?
Note that the working directory of PostgreSQL is $PGDATA so "config_dir"
and files under that can be accessed with a relative path.
But a cleanup somewhere is needed to remove the stale temp files.
I think it's enough at postmaster startup. A machine that's crashing
so often that the presence of the stale temp files causes out of disk
errors would surely be noticed much earlier.
In any approach, there will be chance that temp files will remain if server
crashes during this command execution
which can lead to collision of temp file name next time user tries to use SET
persistent command.
mkstemp() replaces the "XXXXXX" suffix with a unique hexadecimal suffix
so there will be no collision.
An appropriate error will be raised and user manually needs to delete that file.
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.
As far as that goes, %m is appropriate in elog/ereport (which contain
special support for it), but not anywhere else.
In the patch, it's used in ereport, so I assume it is safe and patch doesn't
need any change for %m.
OK.
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
