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

On 8/6/19 11:16 AM, Stephen Frost wrote:
> 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:
>>>>
>>>> + <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