Re: Proposal for Allow postgresql.conf values to be changed via SQL [review] - Mailing list pgsql-hackers

On Monday, May 27, 2013 4:17 PM Amit Kapila wrote:
> On Wednesday, April 03, 2013 11:55 AM Amit Kapila wote:
> > On Tuesday, April 02, 2013 9:49 PM Peter Eisentraut wrote:
> 

There are 2 options to proceed for this patch for 9.4

1. Upload the SET PERSISTENT syntax patch for coming CF by fixing existing
review comments
2. Implement new syntax ALTER SYSTEM as proposed in below mail

Could you suggest me what could be best way to proceed for this patch?

> This feature was discussed for 9.3, but couldn't get committed.
> History for this patch is that in the last, Peter Eisentraut has given
> quite a few review comments, most of which I have closed in the patch
> attached.
> 
> Apart from that Robert and Tom intended to use ALTER SYSTEM or variant
> of ALTER rather than SET. Syntax can be as below:
> 
> ALTER SYSTEM SET configuration_parameter {TO | =} {value, | 'value'};
> 
> Some Comments of Peter E which needs to be discussed before changing
> are as
> below:
> 
> 1) Name of directory should be postgresql.conf.d rather than config.
> 2) Name of the file persistent.auto.conf should not have auto word.
> 
> I have not modified the code, as these had been previously discussed in
> this mail chain and as a conclusion of that, I had kept the current
> names for file and directory.
> 
> > > I'm going to ignore most of the discussion that led up to this and
> > give
> > > this patch a fresh look.
> >
> > Thank you.
> >
> > > + <screen>
> > > + SET PERSISTENT max_connections To 10; </screen>
> > >
> > > The To should probably be capitalized.
> >
> > Okay, shall fix it.
> 
> Changed to
> SET PERSISTENT checkpoint_timeout TO 600
> 
> > > I doubt this example actually works because changing
> max_connections
> > > requires a restart.  Try to find a better example.
> >
> > Any parameter's changed with this command can be effective either
> > after restart or SIGHUP.
> > So I will change it to SIGHUP parameter.
> >
> > > It's weird that SET LOCAL and SET SESSION actually *set* the value,
> > and
> > > the second key word determines how long the setting will last.  SET
> > > PERSISTENT doesn't actually set the value.  I predict that this
> will
> > be
> > > a new favorite help-it-doesn't-work FAQ.
> > >
> > >       <varlistentry>
> > >        <term><literal>SCHEMA</literal></term>
> > >        <listitem>
> > > !       <para><literal>SET [ PERSISTENT ] SCHEMA
> > > '<replaceable>value</>'</> is an alias for
> > >          <literal>SET search_path TO <replaceable>value</></>.
> Only
> > > one
> > >          schema can be specified using this syntax.
> > >         </para>
> > >
> > > I don't think [ PERSISTENT ] needs to be added in these and similar
> > > snippets.  We don't mention LOCAL etc. here either.
> >
> > Agreed, shall fix this.
> 
> Removed from all similar places.
> 
> > > --- 34,41 ----
> > >   <filename>postgresql.conf</filename>,
> > > <filename>pg_hba.conf</filename>, and
> > >   <filename>pg_ident.conf</filename> are traditionally stored in
> > >   <varname>PGDATA</> (although in
> > <productname>PostgreSQL</productname>
> > > 8.0 and
> > > ! later, it is possible to place them elsewhere). By default the
> > > directory config is stored ! in <varname>PGDATA</>, however it
> > > should be kept along with <filename>postgresql.conf</filename>.
> > >   </para>
> > >
> > >   <table tocentry="1" id="pgdata-contents-table">
> > >
> > > This chunk doesn't make any sense to me.  "Should" is always
> tricky.
> > > Why should I, and why might I not?
> >
> > I mean to say here, that user needs to move config directory and its
> > contents along with postgresql.conf.
> > Shall we change as below:
> > By default config directory is stored in PGDATA, however it needs to
> > be kept along with postgresql.conf
> 
> Changed as proposed above.
> 
> >
> > >   <row>
> > > +  <entry><filename>config</></entry>
> > > +  <entry>Subdirectory containing automatically generated
> > configuration
> > > files</entry>
> > > + </row>
> > > +
> > > + <row>
> > >    <entry><filename>base</></entry>
> > >    <entry>Subdirectory containing per-database
> subdirectories</entry>
> > >   </row>
> > >
> > > Only automatically generated ones?
> >
> > No, any other files can also be present.
> > How about change it as :
> > Subdirectory containing generated configuration files.
> > Any other suggestions?
> 
> Changed as above.
> 
> > This new directory's will be used to place generated files,
> >
> > >         COPY_STRING_FIELD(name);
> > >         COPY_NODE_FIELD(args);
> > >         COPY_SCALAR_FIELD(is_local);
> > > +       COPY_SCALAR_FIELD(is_persistent);
> > >
> > >         return newnode;
> > >   }
> > >
> > > I suggest changing is_local into a new trivalued field that stores
> > > LOCAL or SESSION or PERSISTENT.
> > >
> > >                                         n->is_local = false;
> > >                                         $$ = (Node *) n;
> > >                                 }
> >
> > Okay, I will change it.
> >
> > > +                       | SET PERSISTENT set_persistent
> > > +                               {
> > > +                                       VariableSetStmt *n = $3;
> > > +                                       n->is_persistent = true;
> > > +                                       $$ = (Node *) n;
> > > +                               }
> > >                 ;
> > >
> > >   set_rest:
> > >
> > > Why can't you use SET PERSISTENT set_rest?
> >
> > As SET PERSISTENT cannot be used with some of syntaxes, example
> > (SESSION AUTHORIZATION) and also it is not supportted inside
> > transaction blocks.
> >
> > >
> > > *** a/src/backend/replication/basebackup.c
> > > --- b/src/backend/replication/basebackup.c
> > > ***************
> > > *** 755,760 **** sendDir(char *path, int basepathlen, bool
> sizeonly)
> > > --- 755,766 ----
> > >
> strlen(PG_TEMP_FILE_PREFIX))
> > ==
> > > 0)
> > >                         continue;
> > >
> > > +               /* skip auto conf temporary file */
> > > +               if (strncmp(de->d_name,
> > > +                                       PG_AUTOCONF_FILENAME ".",
> > > +
> > > + sizeof(PG_AUTOCONF_FILENAME))
> > > == 0)
> > > +                       continue;
> > > +
> > >
> > > Maybe pg_basebackup should be taught to ignore certain kinds of
> > > temporary files in general.  The file name shouldn't be hardcoded
> > into
> > > pg_basebackup.  This would effectively make the configuration file
> > > naming scheme part of the replication protocol.  See other thread
> > about
> > > pg_basebackup client/server compatibility.  This needs to be
> > > generalized.
> >
> > As we are just trying to ignore the file during backup, why it should
> > effect replication protocol?
> > I mean protocol should be effected, if try to send something new or
> > don't send what is expected on the other side.
> > Also the same is done for team and backup label file.
> >
> > > (Side thought: Does pg_basebackup copy editor temp and backup
> > > files?)
> >
> > Currently pg_basebackup doesn't copy temp or backup files. The check
> I
> > made in code is similar to check for those two.
> >
> > >
> > > +           ereport(elevel,
> > > +               (errmsg("Configuration parameters changed with SET
> > > PERSISTENT command before start of server "
> > > +                       "will not be effective as \"%s\"  file is
> > > + not
> > > accessible.", PG_AUTOCONF_FILENAME)));
> > >
> > > I'm having trouble interpreting this: How can you change something
> > with
> > > SET PERSISTENT before the server starts?
> >
> > This is for the case when somebody starts the server second time, or
> i
> > can say restart server Example 1. Startup Server, Connect client 2.
> > Change some settings with SET PERSISTENT 3. Shutdown server 4. Start
> > Server; at this step if we don't find the file, the above error
> > message will appear.
> >
> > > Also: errmsg messages should start with a lowercase letter and
> > > should generally be much shorter.  Please review other cases in
> your
> > > patch
> > as
> > > well.
> >
> > Okay. I shall take care of it.
> 
> Done, all error messages started with lower case letter. Apart from
> above message, all other error messages are short, do you feel any
> other needs to be shortened.
> 
> > > +   appendStringInfoString(&buf, "# Do not edit this file manually!
> "
> > > +                      "It is overwritten by the SET PERSISTENT
> > command
> > > \n");
> > >
> > > There is some punctuation or something missing at the end.
> > >
> > > I suggest you break this into two lines.  It's pretty long.
> >
> > Okay. I shall take care of it.
> 
> 
> Done, broken into 2 lines.
> 
> >
> > >
> > > I think the naming of these files is suboptimal:
> > >
> > > + #define PG_AUTOCONF_DIR                       "config"
> > >
> > > "config" would seem to include pg_hba.conf and others.  Or
> > > postgresql.conf for that matter.  Maybe I should move
> > > postgresql.conf into config/.  Another great new FAQ.  The normal
> > > convention for this is "postgresql.conf.d".  I don't see any reason
> > > not to use that.  One reason that was brought up is that this
> > > doesn't match other things currently in PGDATA, but (a) actually it
> > > does (hint:
> > postgresql.conf),
> > > and (b) neither does "config".
> >
> > Reason was that "postgresql.conf.d" or "auto.conf.d" doesn't match
> > existing folder name, whereas config can have some similarity with
> > global directory as far as I can see.
> >
> > However changing name is not a big matter, it's more about for which
> > name there is more consensus.
> >
> > > + #define PG_AUTOCONF_FILENAME          "persistent.auto.conf"
> > >
> > > "auto" sounds like the result of an automatic tuning tool.  Why not
> > > just persistent.conf.  Or set-persistent.conf.  That makes the
> > > association clear enough.
> >
> >
> > The 'auto' keyword is kept to make it different from others, as it
> > will be generated by Command.
> > It can give indication to user that this is not similar to other conf
> > files which he can directly edit.
> >
> > > I don't know why the concept of a configuration directory is being
> > > introduced here.  I mean, I'm all for that, but it seems completely
> > > independent of this.  Some people wanted one setting per file, but
> > > that's not what is happening here.  The name of the configuration
> > file
> > > is hardcoded throughout, so there is no value in reading all files
> > from
> > > a directory instead of just that one file.
> > >
> > > There is a lot of confusion throughout the patch about whether this
> > > config directory is for automatic files only or you can throw your
> > own
> > > stuff in there at will.  Another great FAQ.  And then the whole
> > > dance about parsing postgresql.conf to make sure the include_dir is
> > > not removed (=> FAQ).  Does it care whether it's at the beginning
> or
> > > the end?  Should it?
> >
> > This will also work as per existing behavior of #include_dir, which
> > means any configuration parameter values specified after this
> > #include_dir will override the values set by SET PERSISTENT
> statement.
> > The same is mentioned as WARNING just before this include directive.
> >
> > > How does all of this work when you move the configuration files to,
> > > say, /etc/, but want to keep the SET PERSISTENT stuff under
> $PGDATA?
> > > Lots of moving parts there.
> >
> > He needs to move config directory along with postgresql.conf.
> >
> > > I think this could be much easier if you just read
> > > $PGDATA/persistent.conf or whatever if present.  There is perhaps
> an
> > > analogy to be found with the proposed new recovery.conf handling
> > (e.g.,
> > > always read if present after all other postgresql.conf processing
> or
> > > something like that).
> >
> > In current mechanism, user can revert to old usage (edit every thing
> > in
> > postgresql.conf) just by commenting or removing include_dir =
> 'config'
> > in postgresql.conf.
> > OTOH if we just try to read if present mechanism, we might not be
> able
> > to tell to user that his file is missing and if he has made any
> > changes by SET PERSISTENT they will not reflect after this startup.
> >
> > > I'm thinking, this feature is ultimately intended to make things
> > > simpler for average users.  But I see so many traps here.  It needs
> > > to be simpler yet, and have less moving parts.
> > >
> > > I'd almost like an option to turn this off for a server, because it
> > has
> > > so much potential for confusion.
> >
> > If user comments or removes below line in postgresql.conf, then this
> > feature will be OFF include_dir = 'config'
> 
> Do you think above way to switch OFF the feature is acceptable or do
> you have any other idea?
> 
> 
> Suggestions/Feedback?

With Regards,
Amit Kapila.