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

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.
* 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?
* set_config_file should elog(ERROR), not return on an unhandled
setstmt->kind
* why are you creating AutoConfFileName if its not stat'able? It seems
better to simply skip parsing the old file in that case
* 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.
* the write sequence should be:
* fsync(tempfile)
* fsync(directory)
* rename(tempfile, configfile)
* fsync(configfile)
* fsync(directory)
* write_auto_conf_file should probably escape quoted values?
* 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.
* replace_auto_config_file and surrounding functions need more comments
in the header
* 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).

I think this patch is a good bit away of being ready for committer...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services