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

On Tuesday, April 02, 2013 9:49 PM Peter Eisentraut wrote:
> 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.

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

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

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

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.

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

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

With Regards,
Amit Kapila.