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

On 8/6/19 11:16 AM, Stephen Frost wrote:
 > Greetings,
 >
 > * Ian Barwick (ian.barwick@2ndquadrant.com) wrote:
 >> On 8/6/19 9:52 AM, Stephen Frost wrote:> Greetings,
 >>> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
 >>>>
 >>>> +    <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...

Ah, got it, I thought you were talking about the ALTER SYSTEM behaviour.

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

True. (Switches brain on)... Ah yes, with the patch previously provided
by Tom, it seems to be just a case of replacing "strcmp" with "guc_name_compare"
to match the existing string; the name will be rewritten with the value provided
to ALTER SYSTEM, which will be normalized to lower case anyway.

Tweaked version attached.

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

This is just saying what ALTER SYSTEM will do, which IMHO we should describe
somewhere. Initially when I stated working with pg.auto.conf I had
my application append a comment line to show where the entries came from,
but not having any idea how pg.auto.conf was modified at that point, I was
wondering why the comment subsequently disappeared. Perusing the source code has
explained that for me, but would be mighty useful to document that.

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

OK, then we should be saying something like:
- pg.auto.conf may be rewritten at any point and duplicates/comments removed
- the rewrite will occur whenever ALTER SYSTEM is run, removing duplicates
   of the parameter being modified and any comments
- external utilities may also rewrite it; they may, if they wish, remove
   duplicates and comments

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

But at this point ALTER SYSTEM is the only thing which is known to modify
the file, with known effects. If in a future release something else is
added, the documentation can be updated as appropriate.


Regards

Ian Barwick


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