From: Stephen Frost <sfrost@snowman.net>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>
Cc: Ian Barwick <ian.barwick@2ndquadrant.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions
Date: Mon, 5 Aug 2019 20:52:26 -0400
Message-ID: <20190806005225.GM29202@tamriel.snowman.net> (raw)
In-Reply-To: <32103.1565032053@sss.pgh.pa.us>
References: <20190802230011.dqokdhjptykopxxc@development>
<20190805142139.GW29202@tamriel.snowman.net>
<CA+Tgmoatso2sR=_LSSR443YjAfvQ48zpbYYVYQUHVMSoxsrJ9g@mail.gmail.com>
<20190805172935.GK29202@tamriel.snowman.net>
<CA+TgmoZmSQCta7eN_Y2+U4LthMxcutGB+YXs+0-i6ND2JY0TRA@mail.gmail.com>
<30179.1565029753@sss.pgh.pa.us>
<CA+TgmoYEQyLE8Mtgko9-WSVWm=hLNGi3FO4xNLTrLpwi2GgPHw@mail.gmail.com>
<30823.1565030571@sss.pgh.pa.us>
<CA+TgmobRpgiqCZ_FMUeDW4RM1W+RzT4HA2xSQy=QMcfv=X47jw@mail.gmail.com>
<32103.1565032053@sss.pgh.pa.us>
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
Attachments:
signature.asc (application/pgp-signature, 818B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: sfrost@snowman.net, tgl@sss.pgh.pa.us, robertmhaas@gmail.com, tomas.vondra@2ndquadrant.com, amit.kapila16@gmail.com, ian.barwick@2ndquadrant.com
Subject: Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions
In-Reply-To: <20190806005225.GM29202@tamriel.snowman.net>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox