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

On 8/6/19 9:52 AM, Stephen Frost wrote:> 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.

+1.

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

That section applies to all configuration files, the removal is specific
to pg.auto.conf so better mentioned further down.

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

FWIW, as the file is rewritten each time, *all* comments are removed
anyway (the first two comment lines in the file with the warning
are added when the new version of the file is written().

> and parameters may be
> lower-cased." (istr that last bit being true too but I haven't checked
> lately).

Ho-hum, they won't be lower-cased, instead currently they just won't be
overwritten if they're present in pg.auto.conf, which is a slight
eccentricity, but not actually a problem with the current code as the new value
will be written last. E.g.:

$ cat postgresql.auto.conf
# Do not edit this file manually!
# It will be overwritten by the ALTER SYSTEM command.
DEFAULT_TABLESPACE = 'space_1'

postgres=# ALTER SYSTEM SET default_tablespace ='pg_default';
ALTER SYSTEM

$ cat postgresql.auto.conf
# Do not edit this file manually!
# It will be overwritten by the ALTER SYSTEM command.
DEFAULT_TABLESPACE = 'space_1'
default_tablespace = 'pg_default'

I don't think that's worth worrying about now.

My suggestion for the paragaph in question:

<para>
External tools which need to write configuration settings (e.g. for replication)
where it's essential to ensure these are read last (to override versions
of these settings present in other configuration files), may append settings to
<filename>postgresql.auto.conf</filename>. It is not recommended to do this while
the server is running, since a concurrent <command>ALTER SYSTEM</command>
command could overwrite such changes. Note that a subsequent
<command>ALTER SYSTEM</command> will cause <filename>postgresql.auto.conf</filename>,
to be rewritten, removing any duplicate versions of the setting altered, and also
any comment lines present.
</para>

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

This is all on the same page anyway.

Regards

Ian Barwick

--
Ian Barwick https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services