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?

Fixed all whitespaces.

> 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.)

Fixed, by running pgindent

> 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.

Fixed, make check passes now.

> Examples in alter_system.sgml have a typo "SYTEM". Same file has "or
> or".

Fixed.

> 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?

Changed to below text:
Name of a settable run-time parameter. Available parameters are documented
in <xref linkend="runtime-config">.

> <refnamediv>
> <refname>ALTER SYSTEM</refname>
> <refpurpose>change a server configuration parameter</refpurpose>
> </refnamediv>
>
> Perhaps "permanently change .."?

Not changed.

>
> 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?

Fixed, removed dot.
>
>
> + 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()?

Okay, changed as per suggestion.

> 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.

Changed by adding new enum AutoConfErrorCause. Now is_config_dir_parsed is
removed from code.
Kindly let me know if this way is better than previous?

Apart from above I have fixed one issue in function
AlterSystemSetConfigFile(), called FreeConfigVariables().

With Regards,
Amit Kapila.