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
thinkanother 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
thetimezone for a function. And why is it possible to persistently set the search path, but not client encoding? Why is
FROMCURRENT 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
thatcase
* Writing the temporary file to .$pid seems like a bad idea, better use one file for that, SET PERSISTENT is protected
byan 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-lineblocks 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
PreventTransactionChainlike 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