Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: Proposal for Allow postgresql.conf values to be changed via SQL [review])

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.