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.