On Mon, Aug 25, 2014 at 1:34 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Wed, Jul 30, 2014 at 9:11 AM, Amit Kapila <amit.kapila16@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
