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

Le mardi 30 juillet 2013 05:27:12, Amit Kapila a écrit :
> 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.

Yes, it just appeared that this part was a significant one in the patch. I
understand that it is not interesting to split now.

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

ok.

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

I probably missed those suggestions.

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

yeah, I vote here for a shorter name, but it changes nothing to the patch.
--
Cédric Villemain +33 (0)6 20 30 22 52
http://2ndQuadrant.fr/
PostgreSQL: Support 24x7 - Développement, Expertise et Formation