On Wed, Dec 18, 2013 at 2:05 PM, Tatsuo Ishii <is...@postgresql.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
alter_system_v13.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