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

On Monday, July 29, 2013 7:15 PM Cédric Villemain wrote:
> Le lundi 29 juillet 2013 13:47:57, Amit Kapila a écrit :
> > On Sunday, July 28, 2013 11:12 AM Amit kapila wrote:
> > > On Friday, July 26, 2013 6:18 PM Tom Lane wrote:
> > >
> > > Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> > > >> The main contention point I see is where conf.d lives; the two
> > > >> options are in $PGDATA or together with postgresql.conf.
> > >
> > > Tom
> > >
> > > >> and Robert, above, say it should be in $PGDATA; but this goes
> > >
> > > against
> > >
> > > >> Debian packaging and the Linux FHS (or whatever that thing is
> > >
> > > called).
> > >
> > > > Ordinarily, if postgresql.conf is not in $PGDATA, it will be
> > >
> > > somewhere
> > >
> > > > that the postmaster does not (and should not) have write
> > > > permissions for. I have no objection to inventiny a conf.d
> > > > subdirectory, I just
> > >
> > > say
> > >
> > > > that it must be under $PGDATA. The argument that this is against
> > > > FHS is utter nonsense, because anything we write there is not
> > > > static configuration, it's just data.
> > > >
> > > > Come to think of it, maybe part of the reason we're having such a
> > >
> > > hard
> > >
> > > > time getting to consensus is that people are conflating the
> "snippet"
> > > > part with the "writable" part? I mean, if you are thinking you
> > > > want system-management tools to be able to drop in configuration
> > > > fragments
> > >
> > > as
> > >
> > > > separate files, there's a case to be made for a conf.d
> > > > subdirectory
> > >
> > > that
> > >
> > > > lives somewhere that the postmaster can't necessarily write. We
> > > > just mustn't confuse that with support for ALTER SYSTEM SET. I
> > > > strongly believe that ALTER SYSTEM SET must not be designed to
> > > > write anywhere outside $PGDATA.
> > >
> > > I think if we can design conf.d separately for config files of
> > > management tools, then it is better to have postgresql.auto.conf to
> > > be in $PGDATA rather than in $PGDATA/conf.d
> > >
> > > Kindly let me know if you feel otherwise, else I will update and
> > > send patch tomorrow.
> >
> > Modified patch to have postgresql.auto.conf in $PGDATA. Changes are
> as
> > below:
> >
> > 1. initdb to create auto file in $PGDATA 2. ProcessConfigFile to open
> > auto file from data directory, special case handling for initdb 3.
> > AlterSystemSetConfigFile function to consider data directory as
> > reference for operating on auto file 4. modified comments in code and
> > docs to remove usage of config directory 5. modified function
> > write_auto_conf_file() such that even if there is no configuration
> > item to write, it should write header message.
> > This is to handle case when there is only one parameter value and
> > user set it to default, before this modification ,it
> > will write empty file.
>
> I just read the patch, quickly.

Thank you for review.

> You may split the patch thanks to validate_conf_option(), however it is
> not a rule on postgresql-hacker.

The review of the core functionality of patch has been done before the
introduction of function
validate_conf_option() in the patch. It was introduced because there
were some common parts in core implementation of AlterSystem and
set_config_option().
I am really not sure, after having multiple round of reviews by reviewers,
it can add
significant value to split it.

> Why not harcode in ParseConfigFp() that we should parse the auto.conf
> file at the end (and/or if USE_AUTO_CONF is not OFF) instead of
> hacking
> ProcessConfigFile() with data_directory ? (data_directory should be set
> at this
> point) ...

No data_directory will not be set by that time incase of initdb, when
ProcessConfigFile()
is called from SelectConfigFiles()

> just thinking, a very convenient way to enable/disable that
> is just to add/remove the include directive in postgresql.conf. So no
> change should be required in ParseConf at all. Except maybe
> AbsoluteConfigLocation which should prefix the path to auto.conf.d with
> data_directory. What I like with the include directive is that Sysadmin
> can define some GUC *after* the auto.conf so he is sure those are not
> 'erased' by auto.conf (or by the DBA).

I think earlier versions have this implementation, but later based on
suggestions, I have changed it to automatic parsing of auto file after
postgresql.conf

> Also, it looks very interesting to stick to an one-file-for-many-GUC
> when we absolutely don't care : this file should (MUST ?) not be edited
> by hand.
> The thing achieve is that it limits the access to ALTER SYSTEM. One
> file per GUC allows to LWlock only this GUC, isn't it ? (and also does
> not require machinery for holding old/new auto GUC, or at least more
> simple).
>
> It also prevent usage of ALTER SYSTEM for a cluster (as in replication)
> because this is not WAL logged. But it can be easier if trying to
> manage only one GUC at a time.
>
> I agree with Tom comment that this file(s) must be in data_directory.
> postgresql.auto.conf is useless, a "data_directory/auto.conf" (.d/ ?)
> is enough.

There were multiple suggestions for names, but I have kept name
postgresql.auto.conf based
on more votes for it and consensus.

With Regards,
Amit Kapila.