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

On Friday, July 19, 2013 1:33 AM Alvaro Herrera wrote:
> Fujii Masao escribió:
> > On Fri, Jul 19, 2013 at 2:45 AM, Josh Berkus <josh@agliodbs.com>
> wrote:
> > > On 07/18/2013 04:25 AM, Amit Kapila wrote:
> > >> On Thursday, July 18, 2013 12:38 AM Josh Berkus wrote:
> > >>> On 07/17/2013 12:01 PM, Alvaro Herrera wrote:
> > >>>> Both of these seem like they would make troubleshooters' lives
> more
> > >>>> difficult.  I think we should just parse the auto file
> automatically
> > >>>> after parsing postgresql.conf, without requiring the include
> > >>> directive
> > >>>> to be there.
> > >>>
> > >>> Wait, I thought the auto file was going into the conf.d
> directory?
> > >>
> > >> Auto file is going into config directory, but will that make any
> difference
> > >> if we have to parse it automatically after postgresql.conf.
> > >
> > > Well, I thought that the whole conf.d directory automatically got
> parsed
> > > after postgresql.conf.  No?
> >
> > No, in the previous patch. We needed to set include_dir to 'config'
> (though
> > that's the default).
>
> I know this has been discussed already, but I want to do a quick
> summary
> of existing proposals, point out their drawbacks, and present a
> proposal
> of my own.  Note I ignore the whole file-per-setting vs.
> one-file-to-rule-them-all, because the latter has already been shot
> down.  So live ideas floated here are:
>
> 1. have a config/postgresql.auto.conf file, require include_dir or
>    include in postgresql.conf
>    This breaks if user removes the config/ directory, or if the user
>    removes the include_dir directive from postgresql.conf.  ALTER
> SYSTEM
>    is in the business of doing mkdir() or failing altogether if the dir
>    is not present.  Doesn't seem friendly.
>
> 2. have a conf.d/ directory, put the file therein; no include or
>    include_dir directive is necessary.
>    I think this is a separate patch and merits its own discussion.
> This
>    might be a good idea, but I don't think that this is the
>    way to implement ALTER SYSTEM.  If users don't want to allow conf.d
>    they can remove it, but that would break ALTER SYSTEM unnecessarily.
>    Since they are separate features it seems to me that they should
>    function independently.
>
> I think we should just put the config directives of ALTER SYSTEM into a
> file, not dir, alongside postgresql.conf; I would suggest
> postgresql.auto.conf.  This file is parsed automatically after
> postgresql.conf, without requiring an "include" directive in
> postgresql.conf.  If the user does not want that file, they can remove
> it; but a future ALTER SYSTEM will create the file again.

> No need to
> parse stuff to find out whether the directory exists, or the file
> exists, or such nonsense.
 I think this will be removed from patch, once we implement automatic
parsing of config/postgresql.auto.conf after parsing postgresql.conf

> I think the only drawback of this is that there's no way to disable
> ALTER SYSTEM (i.e. there's no directory you can remove to prevent the
> thing from working).  But is this a desirable property?  I mean, if we
> want to disallow ALTER SYSTEM I think we should provide a direct way to
> do that, perhaps allow_alter_system = off in postgresql.conf; but I
> don't think we need to provide this, really, at least not in the first
> implementation.
>
> Note that the Debian packaging uses postgres-writable directories for
> its configuration files, so the daemon is always able to create the
> file
> in the config dir.
>
> This seems the simplest way; config tools such as Puppet know they
> always need to consider the postgresql.auto.conf file.


> I think the whole business of parsing the file, and then verifying
> whether the auto file has been parsed, is nonsensical and should be
> removed from the patch.
 Okay, I understood your concern about checking during parsing to  find error whether directory/file exist. In the next
version,I will remove this error. 

With Regards,
Amit Kapila.