Skip site navigation (1) Skip section navigation (2)

Re: Proposal for Allow postgresql.conf values to be changed via SQL [review]

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: (view raw, whole thread or download thread mbox)
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 "", as at a time only one
>     session is allowed to operate.

What's wrong with mkstemp("config_dir/")
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.


> 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

In response to


pgsql-hackers by date

Next:From: Erik RijkersDate: 2013-01-13 10:17:31
Subject: erroneous restore into pg_catalog schema
Previous:From: Pavel StehuleDate: 2013-01-13 06:54:11
Subject: is it bug? - printing boolean domains in sql/xml function

Privacy Policy | About PostgreSQL
Copyright © 1996-2018 The PostgreSQL Global Development Group