Re: ALTER SYSTEM SET command to change postgresql.conf parameters
От | Tatsuo Ishii |
---|---|
Тема | Re: ALTER SYSTEM SET command to change postgresql.conf parameters |
Дата | |
Msg-id | 20131218.173534.489368460725900966.t-ishii@sraoss.co.jp обсуждение исходный текст |
Ответ на | Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: Proposal for Allow postgresql.conf values to be changed via SQL [review]) (Amit Kapila <amit.kapila16@gmail.com>) |
Ответы |
Re: ALTER SYSTEM SET command to change postgresql.conf parameters
(Amit Kapila <amit.kapila16@gmail.com>)
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (Andrew Dunstan <andrew@dunslane.net>) |
Список | pgsql-hackers |
Hi, I have looked into this because it's marked as "ready for committer". > On Tue, Nov 19, 2013 at 2:06 PM, Haribabu kommi > <haribabu.kommi@huawei.com> wrote: >> On 19 November 2013 09:59 Amit Kapila wrote: >>> On Mon, Nov 18, 2013 at 8:31 PM, Haribabu kommi >>> <haribabu.kommi@huawei.com> wrote: >>> > On 18 November 2013 20:01 Amit Kapila wrote: >>> >> > Code changes are fine. >>> >> > If two or three errors are present in the configuration file, it >>> >> prints the last error >> >> Ok fine I marked the patch as ready for committer. > > Thanks for review. > > With Regards, > Amit Kapila. > EnterpriseDB: http://www.enterprisedb.com It looks like working as advertised. Great! However I have noticed a few minor issues. 1) validate_conf_option +/* + * Validates configuration parameter and value, by calling check hook functions + * depending on record's vartype. It validates if the parameter + * value given is in range of expected predefined value for that parameter. + * + * freemem - true indicates memory for newval and newextra will be + * freed in this function, false indicates it will be freed + * by caller. + * Return value: + * 1: the value is valid + * 0: the name or value is invalid + */ +int +validate_conf_option(struct config_generic * record, const char *name, + const char *value, GucSource source, int elevel, + bool freemem, void *newval, void **newextra) Is there any reason for the function returns int as it always returns 0 or 1. Maybe returns bool is better? 2) initdb.c + strcpy(tempautobuf, "# Do not edit this file manually! \n"); + autoconflines[0] = pg_strdup(tempautobuf); + strcpy(tempautobuf, "# It will be overwritten by the ALTER SYSTEM command. \n"); + autoconflines[1] = pg_strdup(tempautobuf); Is there any reason to use "tempautobuf" here? I think we can simply change to this: + autoconflines[0] = pg_strdup("# Do not edit this file manually! \n"); + autoconflines[1] = pg_strdup("# It will be overwritten by the ALTER SYSTEM command. \n"); 3) initdb.c It seems the memory allocated for autoconflines[0] and autoconflines[1] by pg_strdup is never freed. (I think there's similar problem with "conflines" as well, though it was not introduced by the patch). Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese: http://www.sraoss.co.jp
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Christian KruseДата:
Сообщение: Re: Patch: show xid and xmin in pg_stat_activity and pg_stat_replication