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

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@2ndquadrant.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.
You may split the patch thanks to validate_conf_option(), however it is not a
rule on postgresql-hacker.

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

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.

--
Cédric Villemain +33 (0)6 20 30 22 52
http://2ndQuadrant.fr/
PostgreSQL: Support 24x7 - Développement, Expertise et Formation