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.ko...@huawei.com> wrote:
>> On 19 November 2013 09:59 Amit Kapila wrote:
>>> On Mon, Nov 18, 2013 at 8:31 PM, Haribabu kommi
>>> <haribabu.ko...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to