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

On Thursday, January 24, 2013 1:56 AM Andres Freund wrote:
> On 2013-01-22 12:32:07 +0000, Amit kapila wrote:
> > This closes all comments raised till now for this patch.
> > Kindly let me know if you feel something is missing?
> 
> I am coming late to this patch, so bear with me if I repeat somethign
> said elsewhere.

Thanks for looking at patch.
> Review comments of cursory pass through the patch:
> * most comments are hard to understand. I know the problem of that
>   being hard for a non-native speaker by heart, but I think another
> pass
>   over them would be good thing.

I shall give another pass to improve the comments in code.

> * The gram.y changes arround set_rest_(more|common) seem pretty
> confused
>   to me. 

>E.g. its not possible anymore to set the timezone for a  function. 

What do you exactly mean by this part of comment.

> And why is it possible to persistently set the search path,
>   but not client encoding? Why is FROM CURRENT in set_rest_more?

I think the main reason was some of the commands can work only in
transaction 
and as SET Persistent cannot work inside transaction block, so I had kept
some seggregation. 
I shall reply you the exact reason in separate mail.

> * set_config_file should elog(ERROR), not return on an unhandled
>   setstmt->kind

Shall fix it.

> * why are you creating AutoConfFileName if its not stat'able? It seems
>   better to simply skip parsing the old file in that case

Sure, it's better to handle as you are suggesting.

> * Writing the temporary file to .$pid seems like a bad idea, better use
>   one file for that, SET PERSISTENT is protected by an exclusive lock
>   anyway.
 I think we can use one temporary file, infact that was one of the ways I
have asked in one of the previous mails.  However Tom and Zoltan felt this is better way to do it.  I can think of one
reasonwhere it will be better to have .$pid, and that
 
is if some one has opened the  file manually, then all other sessions can fail (in WINDOWS). Infact this
is one of test Zoltan had performed.


> * the write sequence should be:
>   * fsync(tempfile)
>   * fsync(directory)
>   * rename(tempfile, configfile)
>   * fsync(configfile)
>   * fsync(directory)

Why do we need fsync(directory) and fsync(configfile)?

> * write_auto_conf_file should probably escape quoted values? Can you please elaborate more, I am not able to
understandyour point?
 

> * coding style should be adhered to more closesly, there are many
>   if (pointer) which should be if (pointer != NULL), 

Are you pointing in function  validate_conf_option(const char *name, char
*value)  for below usage:  +                                 if (value)

> single-line blocks
>   enclosed in curlies which shouldn't, etc.
Shall fix.

> * replace_auto_config_file and surrounding functions need more comments
>   in the header
 Sure, shall add more comments in function header of following functions:  validate_conf_option()
RemoveAutoConfTmpFiles() write_auto_conf_file()  replace_auto_config_value()
 

> * the check that prevents persistent SETs in a transaction should
> rather
>   be in utility.c and use PreventTransactionChain like most of the
>   others that need to do that (c.f. T_CreatedbStmt).
 We can move the check in utility.c, but if we use PreventTransactionChain,
then it will be disallowed  in functions as well. But the original idea was to not support in
transaction blocks only.  So I feel use of current function IsTransactionBlock() should be okay.

With Regards,
Amit Kapila.