Re: Proposal for Allow postgresql.conf values to be changed via SQL [review]

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

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.