Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: Proposal for Allow postgresql.conf values to be changed via SQL [review])

On Sat, Nov 16, 2013 at 4:35 PM, Haribabu kommi
<haribabu(dot)kommi(at)huawei(dot)com> wrote:
> On 16 November 2013 09:43 Amit Kapila wrote:
>> On Fri, Nov 15, 2013 at 10:18 PM, Peter Eisentraut <peter_e(at)gmx(dot)net>
>> wrote:
>> > On 11/14/13, 2:50 AM, Amit Kapila wrote:
>> >> Find the rebased version attached with this mail.
>> >
> Please find review comments:
>
> + * ALTER SYSTEM SET
> + *
> + * Command to edit postgresql.conf
> + *****************************************************************************/
>
> I feel it should be "Command to change the configuration parameter"
> because this command is not edits the postgresql.conf file.

Changed the comment, but I think it is better to use persistently
in line suggested by you, so I have done that way.

> ereport(ERROR,
> (errcode(ERRCODE_CONFIG_FILE_ERROR),
> errmsg("configuration file \"%s\" contains errors",
> - ConfigFileName)));
> + ErrorConfFile)));
>
> The ErrorConfFile prints "postgresql.auto.conf" only if there is any parsing problem
> with postgresql.auto.conf otherwise it always print "postgresql.conf" because of any other error.

Changed to ensure ErrorConfFile contains proper config file name.
Note: I have not asssigned file name incase of error in below loop,
as file name in gconf is NULL in most cases and moreover this loops
over
guc_variables which doesn't contain values/parameters from
auto.conf. So I don't think it is required to assign ErrorConfFile in
this loop.

ProcessConfigFile(GucContext context)
{
..
for (i = 0; i < num_guc_variables; i++)
{
struct config_generic *gconf = guc_variables[i];

..
}

>
> + * A stale temporary file may be left behind in case we crash.
> + * Such files are removed on the next server restart.
>
> The above comment is wrong, the stale temporary file will be used
> in the next ALTER SYSTEM command. I didn't find any code where it gets
> deleted on the next server restart.

Removed the comment from top of function and modified the comment
where file is getting opened.

>
> if any postmaster setting which are set by the alter system command which
> leads to failure of server start, what is the solution to user to proceed
> further to start the server. As it is mentioned that the auto.conf file
> shouldn't be edited manually.

Yeah, but in case of emergency user can change it to get server
started. Now the question is whether to mention it in documentation, I
think we can leave this decision to committer. If he thinks that it is
better to document then I will update it.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com