On Sat, Nov 16, 2013 at 4:35 PM, Haribabu kommi <haribabu.ko...@huawei.com> wrote: > On 16 November 2013 09:43 Amit Kapila wrote: >> On Fri, Nov 15, 2013 at 10:18 PM, Peter Eisentraut <pete...@gmx.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
alter_system_v12.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
