On Mon, Aug 25, 2014 at 1:34 PM, Amit Kapila <amit.kapil...@gmail.com> wrote: > On Wed, Jul 30, 2014 at 9:11 AM, Amit Kapila <amit.kapil...@gmail.com> > wrote: >> I have verified the patch and found that it works well for >> all scenario's. Few minor suggestions: >> >> 1. >> ! values to the <filename>postgresql.auto.conf</filename> file. >> ! Setting the parameter to <literal>DEFAULT</literal>, or using the >> ! <command>RESET</command> variant, removes the configuration entry >> from >> >> It would be better if we can write a separate line for RESET ALL >> as is written in case of both Alter Database and Alter Role in their >> respective documentation. >> >> 2. >> ! %type <vsetstmt> generic_set set_rest set_rest_more generic_reset >> reset_rest SetResetClause FunctionSetResetClause >> >> Good to break it into 2 lines. >> >> 3. I think we can add some text on top of function >> AlterSystemSetConfigFile() to explain functionality w.r.t reset all. > > I have updated the patch to address the above points. > > I will mark this patch as "Ready For Committer" as most of the > review comments were already addressed by Vik and remaining > I have addressed in attached patch.
The patch looks good to me. One minor comment is; probably you need to update the tab-completion code. Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
