Re: ALTER SYSTEM SET command to change postgresql.conf parameters

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