On Wednesday, July 03, 2013 2:58 AM Alvaro Herrera wrote: > 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".
I will fix above issues. > 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> Yes you are right. How about below line, same parameter description as SET command Name of a settable run-time parameter. Available parameters are documented in Chapter 18 > Perhaps "permanently change .."? I am not sure, if adding "permanently" is appropriate here, as one could also interpret it, that after parameter is changed with this command, it can never be changed again. > 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. Below link contains useful information in this regard: http://www.postgresql.org/docs/devel/static/config-setting.html Particularly refer below para in above link (the below text is for your reference to see if above link is useful): The configuration file is reread whenever the main server process receives a SIGHUP signal; this is most easily done by running pg_ctl reload from the command-line or by calling the SQL function pg_reload_conf(). The main server process also propagates this signal to all currently running server processes so that existing sessions also get the new value. Alternatively, you can send the signal to a single server process directly. Some parameters can only be set at server start; any changes to their entries in the configuration file will be ignored until the server is restarted. Invalid parameter settings in the configuration file are likewise ignored (but logged) during SIGHUP processing. > > + /* 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? This was from previous version of patch, now it is not required, I will remove it. > > + 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()? How about changing to below messages ereport(elevel, (errmsg("configuration parameters changed with ALTER SYSTEM command will not be effective") errdetail("file \"%s\" containing configuration parameters is not accessible.", PG_AUTOCONF_FILENAME))); ereport(elevel, (errmsg("configuration parameters changed with ALTER SYSTEM command will not be effective") errdetail("default include directive for \"%s\" folder is not present in postgresql.conf", PG_AUTOCONF_DIR))); > 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. It might not be feasible with current implementation as currently it just note down whether it has parsed file with name postgresql.auto.conf and then in outer function takes decision based on that parameter. However if we change current implementation for knowing whether it has parsed postgresql.auto.conf, then might be it is possible. See in below point one suggestion to achieve the same. > 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. How about changing it in way such that 1. we introduce a new enum ConfigDirParseError (3 values a. config_dir_not_parsed, config_dir_parsed, config_file_not_accessible) and a new static variable confdirerr. 2. variable should be initialized with config_dir_not_parsed in function ParseConfigFile() 3. Now in function ParseConfigDirectory, we compare that passed directory name is "config" , then we set confdirerr = config_dir_parsed 4. Further in function ParseConfigDirectory, check during ReadDir, if it has read file postgresql.auto.conf I think above should handle both error cases a. include directive for config file is commented or not present; b. file postgresql.auto.conf is not accessible With Regards, Amit Kapila. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
