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

From: Stephen Frost <sfrost(at)snowman(dot)net>
To: Ian Barwick <ian(dot)barwick(at)2ndquadrant(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions
Date: 2019-08-06 02:16:16
Message-ID: 20190806021616.GP29202@tamriel.snowman.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Greetings,

* Ian Barwick (ian(dot)barwick(at)2ndquadrant(dot)com) wrote:
> 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.

Ok, fair enough.

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

Whoah- the file is *not* rewritten each time. It's only rewritten each
time by *ALTER SYSTEM*, but that it not the only thing that's modifying
the file. That mistaken assumption is part of what got us into this
mess...

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

Erm, those are duplicates though and we're saying that ALTER SYSTEM
removes those... Seems like we should be normalizing the file to be
consistent in this regard too.

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

I dislike the special-casing of ALTER SYSTEM here, where we're basically
saying that only ALTER SYSTEM is allowed to do this cleanup and that if
such cleanup is wanted then ALTER SYSTEM must be run.

What I was trying to get at is a definition of what transformations are
allowed and to make it clear that anything using/modifying the file
needs to be prepared for and work with those transformations. I don't
think we want people assuming that if they don't run ALTER SYSTEM then
they can depend on duplicates being preserved and such.. and, yes, I
know that's a stretch, but if we ever want anything other than ALTER
SYSTEM to be able to make such changes (and I feel pretty confident that
we will...) then we shouldn't document things specifically about when
that command runs.

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

Ah, ok, fair enough.

Thanks!

Stephen

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2019-08-06 02:24:46 Re: Ought to use heap_multi_insert() for pg_attribute/depend insertions?
Previous Message Ian Barwick 2019-08-06 01:55:16 Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions