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

Thanks to both (Andres and Fujii Masao) of you for review.
Please find the patch attached with this mail which contains fixes for the
comments raised by you.
Kindly confirm if the fixes are okay or if I have missed any point raised by
you.

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.
>
> 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 have changed the comments in most parts of 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. And why is it possible to persistently set the search path,
> but not client encoding? Why is FROM CURRENT in set_rest_more?

client encoding and FROM CURRENT syntax will be allowed in SET PERSISTENT.
Others cannot be moved as they need transaction to set them and they are
even not present in postgresql.conf file

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

Fixed.

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

Fixed.

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

now only one file (postgresql.auto.conf.temp) will be created

> * the write sequence should be:
> * fsync(tempfile)
> * fsync(directory)
> * rename(tempfile, configfile)
> * fsync(configfile)
> * fsync(directory)
to do fsync of directory we need to open the directory and then do the
fsync.
rename is an atomic operation, so it will always point to either new or
old file.
I am just not sure whether to modify the code to handle this scenario.
So for now I have added the comment at that place.

> * write_auto_conf_file should probably escape quoted values?

Fixed. Changed write_auto_conf_file() to handle it.

> * coding style should be adhered to more closesly, there are many
> if (pointer) which should be if (pointer != NULL), single-line blocks
> enclosed in curlies which shouldn't, etc.

Fixed.

> * replace_auto_config_file and surrounding functions need more comments
> in the header

Added more comments.

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

Fixed. Now this command will not work for transaction blocks or Functions.

Fujii Masao Comments
---------------------
> When I removed postgresql.auto.conf and restarted the server, I got
> the following warning message. This is not correct because I didn't
> remove "auto.conf.d" from postgresql.conf. What I removed is only
> postgresql.auto.conf.
>
> WARNING: Default "auto.conf.d" is not present in postgresql.conf.
> Configuration parameters changed with SET PERSISTENT command will not
> be effective.
>

Warning message is changed as per discussion in another mail.

> Is it safe to write something in the directory other than data
> directory via SQL?

As discussed, it is okay to write in config directory, so I have not changed
anything for this.

> The description of RESET PERSISTENT is in the document, but it seems
> not to be implemented.

RESET PERSISTENT description is removed from patch.

> storage.sgml needs to be updated.
> Doesn't auto.conf.d need to be explained in config.sgml?

Updated both storage.sgml and config.sgml

> This warning message implies that the line "include_dir 'auto.conf.d'"
> must not be removed from postgresql.conf? If so, we should warn that in
both document and postgresql.conf.sample?

I have modified the WARNING in postgresql.conf.sample, I have thought of
modifying runtime.sgml but not sure if that is the right place, can you
please suggest which document would be better to have this warning?

With Regards,
Amit Kapila.