Amit Kapila escribió: > I have changed the file name to postgresql.auto.conf and I have changed the > name of SetPersistentLock to AutoFileLock. > > Zoltan, > > Could you please once check the attached Patch if you have time, as now all > the things are resolved except for small doubt in syntax extensibility > which can be changed based on final decision.
There are a bunch of whitespace problems, as you would notice with "git diff --check" or "git show --color". Could you please clean that up? Also, some of the indentation in various places is not right; perhaps you could fix that by running pgindent over the source files you modified (this is easily visible by eyeballing the git diff output; stuff is misaligned in pretty obvious ways.) Random minor other comments: + use of <xref linkend="SQL-ALTER SYSTEM"> this SGML link cannot possibly work. Please run "make check" in the doc/src/sgml directory. Examples in alter_system.sgml have a typo "SYTEM". Same file has "or or". Also in that file, The name of an configuration parameter that exist in <filename>postgresql.conf</filename>. the parameter needn't exist in that file to be settable, right? <refnamediv> <refname>ALTER SYSTEM</refname> <refpurpose>change a server configuration parameter</refpurpose> </refnamediv> Perhaps "permanently change .."? Also, some parameters require a restart, not a reload; maybe we should direct the user somewhere else to learn how to freshen up the configuration after calling the command. + /* skip auto conf temporary file */ + if (strncmp(de->d_name, + PG_AUTOCONF_FILENAME ".", + sizeof(PG_AUTOCONF_FILENAME)) == 0) + continue; What's the dot doing there? + if ((stat(AutoConfFileName, &st) == -1)) + ereport(elevel, + (errmsg("configuration parameters changed with ALTER SYSTEM command before restart of server " + "will not be effective as \"%s\" file is not accessible.", PG_AUTOCONF_FILENAME))); + else + ereport(elevel, + (errmsg("configuration parameters changed with ALTER SYSTEM command before restart of server " + "will not be effective as default include directive for \"%s\" folder is not present in postgresql.conf", PG_AUTOCONF_DIR))); These messages should be split. Perhaps have the "will not be effective" in the errmsg() and the reason as part of errdetail()? Also, I'm not really sure about doing another stat() on the file after parsing failed; I think the parse routine should fill some failure context information, instead of having this code trying to reproduce the failure to know what to report. Maybe something like the SlruErrorCause enum, not sure. Similarly, + /* + * record if the file currently being parsed is postgresql.auto.conf, + * so that it can be later used to give warning if it doesn't parse + * it. + */ + snprintf(Filename,sizeof(Filename),"%s/%s", PG_AUTOCONF_DIR, PG_AUTOCONF_FILENAME); + ConfigAutoFileName = AbsoluteConfigLocation(Filename, ConfigFileName); + if (depth == 1 && strcmp(ConfigAutoFileName, config_file) == 0) + *is_config_dir_parsed = true; this seems very odd. The whole "is_config_dir_parsed" mechanism smells strange to me, really. I think the caller should be in charge of keeping track of this, but I'm not sure. ParseConfigFp() would have no way of knowing, and in one place we're calling that routine precisely to parse the auto file. Thanks, -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers