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

On Wed, Dec 18, 2013 at 2:05 PM, Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:
> Hi,
>
> I have looked into this because it's marked as "ready for committer".
Thank you.
> 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?

No, return type should be bool, I have changed the same in attached patch.

> 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");

You are right, I have changed code as per your suggestion.

> 3) initdb.c
>
> It seems the memory allocated for autoconflines[0] and
> autoconflines[1] by pg_strdup is never freed.

I think, it gets freed in writefile() in below code.

for (line = lines; *line != NULL; line++)
{
if (fputs(*line, out_file) < 0)
{
fprintf(stderr, _("%s: could not write file \"%s\": %s\n"),
progname, path, strerror(errno));
exit_nicely();
}
free(*line);
}

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