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-12 07:42:54
Message-ID: (view raw, whole thread or download thread mbox)
Lists: pgsql-hackers
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 ----

+                       /* Delete the file if exists */
+                       {
+                               char LockFileName[MAXPGPATH];
+                               strlcpy(LockFileName, ConfigFileName, sizeof(LockFileName));
+                               get_parent_directory(LockFileName);
+                               join_path_components(LockFileName, LockFileName, 
+                               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.


>> 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

In response to


pgsql-hackers by date

Next:From: Simon RiggsDate: 2013-01-12 10:15:43
Subject: Re: Performance Improvement by reducing WAL for Update Operation
Previous:From: Hitoshi HaradaDate: 2013-01-12 06:28:15
Subject: Validation in to_date()

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