From: | Boszormenyi Zoltan <zb(at)cybertec(dot)at> |
---|---|
To: | Amit kapila <amit(dot)kapila(at)huawei(dot)com> |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, 'Robert Haas' <robertmhaas(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Proposal for Allow postgresql.conf values to be changed via SQL [review] |
Date: | 2013-01-13 09:15:07 |
Message-ID: | 50F27B1B.70203@cybertec.at |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
2013-01-13 05:49 keltezéssel, Amit kapila írta:
> On Sunday, January 13, 2013 12:41 AM Tom Lane wrote:
> Boszormenyi Zoltan <zb(at)cybertec(dot)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/
From | Date | Subject | |
---|---|---|---|
Next Message | Erik Rijkers | 2013-01-13 10:17:31 | erroneous restore into pg_catalog schema |
Previous Message | Pavel Stehule | 2013-01-13 06:54:11 | is it bug? - printing boolean domains in sql/xml function |