Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions

Greetings,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Mon, Aug 5, 2019 at 2:42 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> I don't think we need to go on about it at great length, but it seems
> >> to me that it'd be reasonable to point out that (a) you'd be well
> >> advised not to touch the file while the postmaster is up, and (b)
> >> last setting wins. Those things are equally true of postgresql.conf
> >> of course, but I don't recall whether they're already documented.
>
> > OK, fair enough.
>
> Concretely, how about the attached?

> (Digging around in config.sgml, I found that last-one-wins is stated,
> but only in the context of one include file overriding another.
> That's not *directly* a statement about what happens within a single
> file, and it's in a different subsection anyway, so repeating the
> info in 19.1.2 doesn't seem unreasonable.)

Agreed.

> diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
> index cdc30fa..f5986b2 100644
> --- a/doc/src/sgml/config.sgml
> +++ b/doc/src/sgml/config.sgml
> @@ -153,6 +153,8 @@ shared_buffers = 128MB
> identifiers or numbers must be single-quoted. To embed a single
> quote in a parameter value, write either two quotes (preferred)
> or backslash-quote.
> + If the file contains multiple entries for the same parameter,
> + all but the last one are ignored.
> </para>

Looking at this patch quickly but also in isolation, so I could be wrong
here, but it seems like the above might be a good place to mention
"duplicate entries and comments may be removed."

> <para>
> @@ -185,18 +187,27 @@ shared_buffers = 128MB
> In addition to <filename>postgresql.conf</filename>,
> a <productname>PostgreSQL</productname> data directory contains a file
> <filename>postgresql.auto.conf</filename><indexterm><primary>postgresql.auto.conf</primary></indexterm>,
> - which has the same format as <filename>postgresql.conf</filename> but should
> - never be edited manually. This file holds settings provided through
> - the <xref linkend="sql-altersystem"/> command. This file is automatically
> - read whenever <filename>postgresql.conf</filename> is, and its settings take
> - effect in the same way. Settings in <filename>postgresql.auto.conf</filename>
> - override those in <filename>postgresql.conf</filename>.
> + which has the same format as <filename>postgresql.conf</filename> but
> + is intended to be edited automatically not manually. This file holds
> + settings provided through the <xref linkend="sql-altersystem"/> command.
> + This file is read whenever <filename>postgresql.conf</filename> is,
> + and its settings take effect in the same way. Settings
> + in <filename>postgresql.auto.conf</filename> override those
> + in <filename>postgresql.conf</filename>.
> + </para>

The above hunk looks fine.

> + <para>
> + External tools might also
> + modify <filename>postgresql.auto.conf</filename>, typically by appending
> + new settings to the end. It is not recommended to do this while the
> + server is running, since a concurrent <command>ALTER SYSTEM</command>
> + command could overwrite such changes.
> </para>

Alternatively, or maybe also here, we could say "note that appending to
the file as a mechanism for setting a new value by an external tool is
acceptable even though it will cause duplicates- PostgreSQL will always
use the last value set and other tools should as well. Duplicates and
comments may be removed when rewriting the file, and parameters may be
lower-cased." (istr that last bit being true too but I haven't checked
lately).

> <para>
> The system view
> <link linkend="view-pg-file-settings"><structname>pg_file_settings</structname></link>
> - can be helpful for pre-testing changes to the configuration file, or for
> + can be helpful for pre-testing changes to the configuration files, or for
> diagnosing problems if a <systemitem>SIGHUP</systemitem> signal did not have the
> desired effects.
> </para>

This hunk looks fine.

Reviewing https://www.postgresql.org/docs/current/config-setting.html
again, it looks reasonably comprehensive regarding the format of the
file- perhaps we should link to there from the "external tools might
also modify" para..? "Tool authors should review <link> to understand
the structure of postgresql.auto.conf".

Thanks!

Stephen