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

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Mon, Aug 5, 2019 at 2:42 PM Tom Lane <tgl@sss.pgh.pa.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