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

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:

>One specific comment about the documentation part of the patch:
>
>***************
>*** 86,92 **** SET [ SESSION | LOCAL ] TIME ZONE { <replaceable class="PARAMETER">timezone</rep
> <application>PL/pgSQL</application> exception block. This behavior
> has been changed because it was deemed unintuitive.
> </para>
>! </note>
> </refsect1>
>
> <refsect1>
>--- 95,101 ----
> <application>PL/pgSQL</application> exception block. This behavior
> has been changed because it was deemed unintuitive.
> </para>
>! </note>
> </refsect1>
>
> <refsect1>
>***************
Fixed the white space comment.

># This includes the default configuration directory included to support
># SET PERSISTENT statement.
>#
># WARNNING: Any configuration parameter values specified after this line
># will override the values set by SET PERSISTENT statement.
>include_dir = 'config_dir'
>
>Note the typo in the above message: WARNNING, there are too many Ns.

Fixed.

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

>I also tried to trigger one of the errors by creating the lock file manually.
>You need an extra space between the "... retry" and "or ...":

Fixed.

>Also, since the SET PERSISTENT patch seems to create a single lock file,
>sizeof(string) (which includes the 0 byte at the end of the string, so it
>matches the whole filename) can be used instead of strlen(string) or
>sizeof(string)-1 that match the filename as a prefix only.
>#define PGAUTOCONFLOCK "postgresql.auto.conf.lock"
+ /* skip lock files used in postgresql.auto.conf edit */
+ if (strncmp(de->d_name,
+ "postgresql.auto.conf.lock",
+ strlen("postgresql.auto.conf.lock")) == 0)

Added a macro and changed the check accordingly.

With Regards,
Amit Kapila.