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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
