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

I'm going to ignore most of the discussion that led up to this and give
this patch a fresh look.

+ <screen>
+ SET PERSISTENT max_connections To 10;
+ </screen>

The To should probably be capitalized.

I doubt this example actually works because changing max_connections
requires a restart. Try to find a better example.

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.

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

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

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;
}
+ | SET PERSISTENT set_persistent
+ {
+ VariableSetStmt *n = $3;
+ n->is_persistent = true;
+ $$ = (Node *) n;
+ }
;

set_rest:

Why can't you use SET PERSISTENT set_rest?

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

(Side thought: Does pg_basebackup copy editor temp and backup files?)

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

Also: errmsg messages should start with a lowercase letter and should
generally be much shorter. Please review other cases in your patch as well.

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

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

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

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?

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.

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

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.