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


On 8/6/19 11:16 AM, Stephen Frost wrote:
> Greetings,
>
> * Ian Barwick (ian.barw...@2ndquadrant.com) wrote:
>> On 8/6/19 9:52 AM, Stephen Frost wrote:> Greetings,
>>> * Tom Lane (t...@sss.pgh.pa.us) wrote:

 +
 + External tools might also
 + modify postgresql.auto.conf, typically by 
appending
 + new settings to the end.  It is not recommended to do this while the
 + server is running, since a concurrent ALTER SYSTEM
 + command could overwrite such changes.

>>>
>>> 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:
>>
>>  
>>   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
>>   postgresql.auto.conf. It is not recommended to do 
this while
>>   the server is running, since a concurrent ALTER 
SYSTEM
>>   command could overwrite such changes. Note that a subsequent
>>   ALTER SYSTEM will cause 
postgresql.auto.conf,
>>   to be rewritten, removing any duplicate versions of the setting 
altered, and also
>>   any comment lines present.
>>  
>
> 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

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

2019-08-05 Thread Michael Paquier

On Mon, Aug 05, 2019 at 10:16:16PM -0400, Stephen Frost wrote:
> * Ian Barwick (ian.barw...@2ndquadrant.com) wrote:
>> On 8/6/19 9:52 AM, Stephen Frost wrote:> Greetings,
>>> * Tom Lane (t...@sss.pgh.pa.us) wrote:
 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.

I have read the latest patch from Tom and I have a suggestion about
this part:
+ and its settings take effect in the same way.  Settings
+ in postgresql.auto.conf override those
+ in postgresql.conf.

It seems to me that we should mention included files here, as any
settings in postgresql.auto.conf override these as well.
--
Michael


signature.asc
Description: PGP signature

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

Greetings,

* Ian Barwick (ian.barw...@2ndquadrant.com) wrote:
> On 8/6/19 9:52 AM, Stephen Frost wrote:> Greetings,
> > * Tom Lane (t...@sss.pgh.pa.us) wrote:
> >> Robert Haas  writes:
> >>> On Mon, Aug 5, 2019 at 2:42 PM Tom Lane  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.
> >>   
> >
> > 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.

> >>   
> >> @@ -185,18 +187,27 @@ shared_buffers = 128MB
> >>In addition to postgresql.conf,
> >>a PostgreSQL data directory contains a 
> >> file
> >>
> >> postgresql.auto.confpostgresql.auto.conf,
> >> - which has the same format as postgresql.conf 
> >> but should
> >> - never be edited manually.  This file holds settings provided through
> >> - the  command.  This file is 
> >> automatically
> >> - read whenever postgresql.conf is, and its 
> >> settings take
> >> - effect in the same way.  Settings in 
> >> postgresql.auto.conf
> >> - override those in postgresql.conf.
> >> + which has the same format as postgresql.conf but
> >> + is intended to be edited automatically not manually.  This file holds
> >> + settings provided through the  
> >> command.
> >> + This file is read whenever postgresql.conf is,
> >> + and its settings take effect in the same way.  Settings
> >> + in postgresql.auto.conf override those
> >> + in postgresql.conf.
> >> +
> >
> > The above hunk looks fine.
> >
> >> +
> >> + External tools might also
> >> + modify postgresql.auto.conf, typically by 
> >> appending
> >> + new settings to the end.  It is not recommended to do this while the
> >> + server is running, since a concurrent ALTER SYSTEM
> >> + command could overwrite such changes.
> >>   
> >
> > 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

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


On 8/6/19 9:52 AM, Stephen Frost wrote:> Greetings,
>
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> Robert Haas  writes:
>>> On Mon, Aug 5, 2019 at 2:42 PM Tom Lane  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.
>>   
>
> 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.

>>   
>> @@ -185,18 +187,27 @@ shared_buffers = 128MB
>>In addition to postgresql.conf,
>>a PostgreSQL data directory contains a file
>>
postgresql.auto.confpostgresql.auto.conf,
>> - which has the same format as postgresql.conf but 
should
>> - never be edited manually.  This file holds settings provided through
>> - the  command.  This file is 
automatically
>> - read whenever postgresql.conf is, and its 
settings take
>> - effect in the same way.  Settings in 
postgresql.auto.conf
>> - override those in postgresql.conf.
>> + which has the same format as postgresql.conf but
>> + is intended to be edited automatically not manually.  This file holds
>> + settings provided through the  
command.
>> + This file is read whenever postgresql.conf is,
>> + and its settings take effect in the same way.  Settings
>> + in postgresql.auto.conf override those
>> + in postgresql.conf.
>> +
>
> The above hunk looks fine.
>
>> +
>> + External tools might also
>> + modify postgresql.auto.conf, typically by 
appending
>> + new settings to the end.  It is not recommended to do this while the
>> + server is running, since a concurrent ALTER SYSTEM
>> + command could overwrite such changes.
>>   
>
> 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:


 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
 postgresql.auto.conf. It is not recommended to do 
this while
 the server is running, since a concurrent ALTER SYSTEM
 command could overwrite such changes. Note that a subsequent
 ALTER

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

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Robert Haas  writes:
> > On Mon, Aug 5, 2019 at 2:42 PM Tom Lane  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.
>  

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

>  
> @@ -185,18 +187,27 @@ shared_buffers = 128MB
>   In addition to postgresql.conf,
>   a PostgreSQL data directory contains a file
>   
> postgresql.auto.confpostgresql.auto.conf,
> - which has the same format as postgresql.conf but 
> should
> - never be edited manually.  This file holds settings provided through
> - the  command.  This file is 
> automatically
> - read whenever postgresql.conf is, and its settings 
> take
> - effect in the same way.  Settings in 
> postgresql.auto.conf
> - override those in postgresql.conf.
> + which has the same format as postgresql.conf but
> + is intended to be edited automatically not manually.  This file holds
> + settings provided through the  command.
> + This file is read whenever postgresql.conf is,
> + and its settings take effect in the same way.  Settings
> + in postgresql.auto.conf override those
> + in postgresql.conf.
> +

The above hunk looks fine.

> +
> + External tools might also
> + modify postgresql.auto.conf, typically by appending
> + new settings to the end.  It is not recommended to do this while the
> + server is running, since a concurrent ALTER SYSTEM
> + command could overwrite such changes.
>  

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

>  
>   The system view
>linkend="view-pg-file-settings">pg_file_settings
> - 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 SIGHUP signal did not 
> have the
>   desired effects.
>  

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  to understand
the structure of postgresql.auto.conf".

Thanks!

Stephen


signature.asc
Description: PGP signature

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

On Mon, Aug 5, 2019 at 3:07 PM Tom Lane  wrote:
> Concretely, how about the attached?

Works for me, for whatever that's worth.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

Robert Haas  writes:
> On Mon, Aug 5, 2019 at 2:42 PM Tom Lane  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.)

regards, tom lane

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.
 
 
 
@@ -185,18 +187,27 @@ shared_buffers = 128MB
  In addition to postgresql.conf,
  a PostgreSQL data directory contains a file
  postgresql.auto.confpostgresql.auto.conf,
- which has the same format as postgresql.conf but should
- never be edited manually.  This file holds settings provided through
- the  command.  This file is automatically
- read whenever postgresql.conf is, and its settings take
- effect in the same way.  Settings in postgresql.auto.conf
- override those in postgresql.conf.
+ which has the same format as postgresql.conf but
+ is intended to be edited automatically not manually.  This file holds
+ settings provided through the  command.
+ This file is read whenever postgresql.conf is,
+ and its settings take effect in the same way.  Settings
+ in postgresql.auto.conf override those
+ in postgresql.conf.
+
+
+
+ External tools might also
+ modify postgresql.auto.conf, typically by appending
+ new settings to the end.  It is not recommended to do this while the
+ server is running, since a concurrent ALTER SYSTEM
+ command could overwrite such changes.
 
 
 
  The system view
  pg_file_settings
- 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 SIGHUP signal did not have the
  desired effects.

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

Greetings,

On Mon, Aug 5, 2019 at 14:43 Tom Lane  wrote:

> Robert Haas  writes:
> > On Mon, Aug 5, 2019 at 2:29 PM Tom Lane  wrote:
> >> I think Stephen is not being unreasonable to suggest that we need some
> >> documentation about what external tools may safely do to pg.auto.conf.
> >> So somebody's got to write that.
>
> > I mean, really?  We're going to document that if you want to add a
> > setting to the file, you can just append it, but that if you find
> > yourself desirous of appending so many settings that the entire disk
> > will fill up, you should maybe reconsider? Perhaps I'm being mean
> > here, but that seems like it's straight out of the
> > blinding-flashes-of-the-obvious department.
>
> 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.

Folks certainly modify postgresql.conf while the postmaster is running
pretty routinely, and we expect them to which is why we have a reload
option, so I don’t think we can say that the auto.conf and postgresql.conf
are to be handled in the same way.

Last setting wins, duplicates should be ignored and may be removed,
comments should be ignored and may be removed, and appending to the file is
acceptable for modifying a value.  I’m not sure how much we really document
the structure of the file itself offhand- back when users were editing it
we could probably be a bit more fast and loose with it, but now that we
have different parts of the system modifying it along with external tools
doing so, we should probably write it down a bit more clearly/precisely.

I suspect the authors of pg_conftool would appreciate that too, so they
could make sure that they aren’t doing anything unexpected or incorrect.

Thanks,

Stephen

>

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

Robert Haas  writes:
> But that's not the problem.  The problem is that ALTER SYSTEM modifies
> the first match instead of the last one, when it's a well-established
> principle that when reading from a PostgreSQL configuration file, the
> system adopts the value from the last match, not the first one. I
> admit that if somebody had thought to document what ALTER SYSTEM was
> doing, that person would probably have also realized that they had
> made a mistake in the code, and then they would have fixed the bug,
> and that would be great.

Well, actually, the existing code is perfectly clear about this:

/* Search the list for an existing match (we assume there's only one) */

That assumption is fine *if* you grant that only ALTER SYSTEM itself
is authorized to write that file.  I think the real argument here
centers around who else is authorized to write the file, and when
and how.

In view of the point you made upthread that we explicitly made
pg.auto.conf a plain text file so that one could recover from
mistakes by hand-editing it, I think it's pretty silly to adopt
a position that external mods are disallowed.

regards, tom lane

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

Greetings,

On Mon, Aug 5, 2019 at 14:38 Robert Haas  wrote:

> On Mon, Aug 5, 2019 at 2:29 PM Tom Lane  wrote:
> > Robert Haas  writes:
> > > All we need to do to resolve this issue is have Tom commit his patch.
> >
> > I think Stephen is not being unreasonable to suggest that we need some
> > documentation about what external tools may safely do to pg.auto.conf.
> > So somebody's got to write that.
>
> I mean, really?  We're going to document that if you want to add a
> setting to the file, you can just append it, but that if you find
> yourself desirous of appending so many settings that the entire disk
> will fill up, you should maybe reconsider? Perhaps I'm being mean
> here, but that seems like it's straight out of the
> blinding-flashes-of-the-obvious department.
>
> If we were going to adopt Stephen's proposed rule that you must remove
> duplicates or be punished later with an annoying WARNING, I would
> agree that it ought to be documented, because I believe many people
> would find that a POLA violation.  And to be clear, I'm not objecting
> to a sentence someplace that says that duplicates in
> postgresql.auto.conf will be ignored and the last value will be used,
> same as for any other PostgreSQL configuration file. However, I think
> it's highly likely people would have assumed that anyway.

The authors and committer for ALTER SYSTEM didn’t.  It’s not uncommon for
us to realize when different people and/or parts of the system make
different assumption about something and they end up causing bugs, we try
to document the “right way” and what expectations one can have.

Also, to be clear, if we document it then I don’t feel we need a WARNING to
be issued- because then it’s expected and handled.

Yes, there was a lot of discussion about how it’d be nice to go further
than documentation and actually provide a facility for tools to use to
modify the file, so we could have the same code being used by pg_basebackup
and ALTER SYSTEM, but the argument was made that we can’t make that happen
for v12.  I’m not sure I agree with that because a lot of the responses
have been blowing up the idea of what amounts to a simple parser/editor of
PG config files (which, as Christoph pointed out, has already been done
externally and I doubt it’s actually all that’s much code) to a full blown
we must validate everything and load every extension’s .so file to make
sure the value is ok, but even so, I’ve backed away from that position and
agreed that a documentation fix would be enough for v12 and hopefully
someone will revisit it in the future and improve on it- at least with the
documentation, there would be a higher chance that they’d get it right and
not end up making different assumptions than those made by other hackers.

Thanks,

Stephen

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

On Mon, Aug 5, 2019 at 2:35 PM Stephen Frost  wrote:
> I dare say that if we had some documentation around what to expect and how to 
> deal with it, for our own tools as well as external ones, then maybe we 
> wouldn’t be in this situation in the first place.  Clearly ALTER SYSTEM and 
> the pg_basebackup modifications had different understands and expectations.

But that's not the problem.  The problem is that ALTER SYSTEM modifies
the first match instead of the last one, when it's a well-established
principle that when reading from a PostgreSQL configuration file, the
system adopts the value from the last match, not the first one. I
admit that if somebody had thought to document what ALTER SYSTEM was
doing, that person would probably have also realized that they had
made a mistake in the code, and then they would have fixed the bug,
and that would be great.

But we have exactly zero need to invent a new set of principles
explaining how to deal with postgresql.auto.conf.  We just need to
make the ALTER SYSTEM code conform to the same general rule that has
been well-established for many years.  The only reason why we're still
carrying on about this 95 messages later is because you're trying to
make an argument that postgresql.auto.conf is a different kind of
thing from postgresql.conf and therefore can have its own novel set of
rules which consequently need to be debated.  IMHO, it's not, it
shouldn't, and they don't.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On Mon, Aug 5, 2019 at 2:42 PM Tom Lane  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.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

Robert Haas  writes:
> On Mon, Aug 5, 2019 at 2:29 PM Tom Lane  wrote:
>> I think Stephen is not being unreasonable to suggest that we need some
>> documentation about what external tools may safely do to pg.auto.conf.
>> So somebody's got to write that.

> I mean, really?  We're going to document that if you want to add a
> setting to the file, you can just append it, but that if you find
> yourself desirous of appending so many settings that the entire disk
> will fill up, you should maybe reconsider? Perhaps I'm being mean
> here, but that seems like it's straight out of the
> blinding-flashes-of-the-obvious department.

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.

regards, tom lane

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

On Mon, Aug 5, 2019 at 2:29 PM Tom Lane  wrote:
> Robert Haas  writes:
> > All we need to do to resolve this issue is have Tom commit his patch.
>
> I think Stephen is not being unreasonable to suggest that we need some
> documentation about what external tools may safely do to pg.auto.conf.
> So somebody's got to write that.

I mean, really?  We're going to document that if you want to add a
setting to the file, you can just append it, but that if you find
yourself desirous of appending so many settings that the entire disk
will fill up, you should maybe reconsider? Perhaps I'm being mean
here, but that seems like it's straight out of the
blinding-flashes-of-the-obvious department.

If we were going to adopt Stephen's proposed rule that you must remove
duplicates or be punished later with an annoying WARNING, I would
agree that it ought to be documented, because I believe many people
would find that a POLA violation.  And to be clear, I'm not objecting
to a sentence someplace that says that duplicates in
postgresql.auto.conf will be ignored and the last value will be used,
same as for any other PostgreSQL configuration file. However, I think
it's highly likely people would have assumed that anyway.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

Greetings,

On Mon, Aug 5, 2019 at 14:29 Tom Lane  wrote:

> Robert Haas  writes:
> > All we need to do to resolve this issue is have Tom commit his patch.
>
> I think Stephen is not being unreasonable to suggest that we need some
> documentation about what external tools may safely do to pg.auto.conf.

I dare say that if we had some documentation around what to expect and how
to deal with it, for our own tools as well as external ones, then maybe we
wouldn’t be in this situation in the first place.  Clearly ALTER SYSTEM and
the pg_basebackup modifications had different understands and expectations.

So somebody's got to write that.  But I agree that we could go ahead
> with the code patch.

I haven’t looked at the code patch at all, just to be clear.  That said, if
you’re comfortable with it and it’s in line with what we document as being
how you handle pg.auto.conf (for ourselves as well as external tools..),
then that’s fine with me.

Thanks,

Stephen

>

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

Robert Haas  writes:
> All we need to do to resolve this issue is have Tom commit his patch.

I think Stephen is not being unreasonable to suggest that we need some
documentation about what external tools may safely do to pg.auto.conf.
So somebody's got to write that.  But I agree that we could go ahead
with the code patch.

(At this point I won't risk doing so before the wrap, though.)

regards, tom lane

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

Greetings,

On Mon, Aug 5, 2019 at 14:11 Andres Freund  wrote:

> On 2019-08-05 13:34:39 -0400, Stephen Frost wrote:
> > * Andres Freund (and...@anarazel.de) wrote:
> > > On 2019-08-05 12:43:24 -0400, Tom Lane wrote:
> > > > On the other hand, people have also opined that they want full error
> > > > checking on the inserted values, and that seems out of reach with
> less
> > > > than a full running system (mumble extensions mumble).
> > >
> > > I think the error checking ought to be about as complete as the one we
> > > do during a normal postmaster startup. Afaict that requires loading
> > > shared_preload_library extensions, but does *not* require booting up
> far
> > > enough to run GUC checks in a context with database access.
> >
> > I'm not following this thread of the discussion.
>
> It's about the future, not v12.

I’m happy to chat about post-v12, certainly. As I said up thread, I get
that we are in this unfortunate situation for v12 and we can do what needs
doing here (where I agree with what Tom said, “a doc patch”- and with fixes
for ALTER SYSTEM to be in line with that doc patch, along with
pg_basebackup, should any changes be needed, of course).

> Where are you thinking this error checking would be happening?
>
> A hypothethical post v12 tool that can set config values with as much
> checking as feasible. The IMO most realistic tool to do so is postmaster
> itself, similar to it's already existing -C.  Boot it up until
> shared_preload_libraries have been processed, run check hook(s) for the
> new value(s), change postgresql.auto.conf, shutdown.

That’s a nice idea but I don’t think it’s really necessary and I’m not sure
how useful this level of error checking would end up being as part of
pg_basebackup.

I can certainly see value in a tool that could be run to verify a
postgresql.conf+auto.conf is valid to the extent that we are able to do so,
since that could, ideally, be run by the init script system prior to a
restart to let the user know that their restart is likely to fail.  Having
that be some option to the postmaster could work, as long as it is assured
to not do anything that would upset a running PG instance (like, say, try
to allocate shared buffers).

> You're not suggesting that pg_basebackup perform this error checking
> > after it modifies the file, are you..?
>
> Not at the moment, at least.

Since pg_basebackup runs, typically, on a server other than the one that PG
is running on, it certainly would have to have a way to at least disable
anything that caused it to try and load libraries on the destination side,
or do anything else that required something external in order to validate-
but frankly I don’t think it should ever be loading libraries that it has
no business with, not even if it means that the error checking of the
postgresql.conf would be wonderful.

Thanks,

Stephen

>

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

On Mon, Aug 5, 2019 at 1:29 PM Stephen Frost  wrote:
> No, that isn't the point of the controversy nor does it relate, at all,
> to what you quoted above, so I don't think there's much value in
> responding to the discussion about WARNING or not that you put together
> below.

Well, if that's not what we're arguing about, then what the heck are
we arguing about?

All we need to do to resolve this issue is have Tom commit his patch.
The set of people who are objecting to that is either {} or {Stephen
Frost}.  Even if it's the latter, we should just proceed, because
there are clearly enough votes in favor of the patch to proceed,
including 2 from RMT members, and if it's the former, we should
DEFINITELY proceed.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

2019-08-05 Thread Andres Freund

Hi,

On 2019-08-05 13:34:39 -0400, Stephen Frost wrote:
> * Andres Freund (and...@anarazel.de) wrote:
> > On 2019-08-05 12:43:24 -0400, Tom Lane wrote:
> > > On the other hand, people have also opined that they want full error
> > > checking on the inserted values, and that seems out of reach with less
> > > than a full running system (mumble extensions mumble).
> > 
> > I think the error checking ought to be about as complete as the one we
> > do during a normal postmaster startup. Afaict that requires loading
> > shared_preload_library extensions, but does *not* require booting up far
> > enough to run GUC checks in a context with database access.
> 
> I'm not following this thread of the discussion.

It's about the future, not v12.


> Where are you thinking this error checking would be happening?

A hypothethical post v12 tool that can set config values with as much
checking as feasible. The IMO most realistic tool to do so is postmaster
itself, similar to it's already existing -C.  Boot it up until
shared_preload_libraries have been processed, run check hook(s) for the
new value(s), change postgresql.auto.conf, shutdown.


> You're not suggesting that pg_basebackup perform this error checking
> after it modifies the file, are you..?

Not at the moment, at least.


Greetings,

Andres Freund

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

Greetings,

* Andres Freund (and...@anarazel.de) wrote:
> On 2019-08-05 12:43:24 -0400, Tom Lane wrote:
> > On the other hand, people have also opined that they want full error
> > checking on the inserted values, and that seems out of reach with less
> > than a full running system (mumble extensions mumble).
> 
> I think the error checking ought to be about as complete as the one we
> do during a normal postmaster startup. Afaict that requires loading
> shared_preload_library extensions, but does *not* require booting up far
> enough to run GUC checks in a context with database access.

I'm not following this thread of the discussion.

You're not suggesting that pg_basebackup perform this error checking
after it modifies the file, are you..?

Where are you thinking this error checking would be happening?

Thanks,

Stephen


signature.asc
Description: PGP signature

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

Greetings,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Mon, Aug 5, 2019 at 10:21 AM Stephen Frost  wrote:
> > Just to be clear, I brought up this exact concern back in *November*:
> >
> > https://www.postgresql.org/message-id/20181127153405.GX3415%40tamriel.snowman.net
> >
> > And now because this was pushed forward and the concerns that I raised
> > ignored, we're having to deal with this towards the end of the release
> > cycle instead of during normal development.
> 
> I disagree. 

It's unclear what you're disagreeing with here as the below response
doesn't seem to discuss the question about if these issues were brought
up and pointed out previously, nor about if I was the one who raised
them, nor about if we're towards the end of the release cycle.

> My analysis is that you're blocking a straightforward bug
> fix because we're not prepared to redesign the world to match your
> expectations. The actual point of controversy at the moment, as I
> understand it, is this: if the backend, while rewriting
> postgresql.auto.conf, discovers that it contains duplicates, should we
> (a) give a WARNING or (b) not?

No, that isn't the point of the controversy nor does it relate, at all,
to what you quoted above, so I don't think there's much value in
responding to the discussion about WARNING or not that you put together
below.

Thanks,

Stephen

signature.asc
Description: PGP signature

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

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Robert Haas  writes:
> > On Mon, Aug 5, 2019 at 12:25 PM Tom Lane  wrote:
> >> Perhaps as a future improvement, but it's not happening for v12,
> >> at least not unless you accept "use ALTER SYSTEM in a standalone
> >> backend" as a usable answer.
> 
> > I'm not sure that would even work for the cases at issue ... because
> > we're talking about setting up recovery parameters, and wouldn't the
> > server run recovery before it got around to do anything with ALTER
> > SYSTEM?
> 
> Yeah, good point.  There are a lot of other cases where you really
> don't want system startup to happen, too.  On the other hand,
> people have also opined that they want full error checking on
> the inserted values, and that seems out of reach with less than
> a full running system (mumble extensions mumble).

There have been ideas brought up about some way to provide "full
validation" but I, at least, don't recall seeing anyone actually say
that they *want* that- just different people suggesting that it could be
done.

I agree that full validation is a pipe dream for this kind of system and
isn't what I was intending to suggest at any point.

> In the end, I think I don't buy Stephen's argument that there should
> be a one-size-fits-all answer.  It seems entirely reasonable that
> we'll have more than one way to do it, because the constraints are
> different depending on what use-case you think about.

This doesn't seem to me, at least, to be an accurate representation of
my thoughts on this- there could be 15 different ways to modify the
file, but let's have a common set of code to provide those ways instead
of different code between the backend ALTER SYSTEM and the frontend
pg_basebackup (and if we put it in the common library that we already
have for sharing code between the backend and the frontend, and which we
make available for external tools, then those external tools could use
those methods in the same way that we do).

I'm happy to be told I'm wrong, but as far as I know, there's nothing in
appending to the file or removing duplicates that actually requires
validation of the values which are included in order to apply those
operations correctly.

I'm sure I'll be told again about how we can't do this for 12, and I do
appreciate that, but it's because we ignored these issues during
development that we're here and that's really just not something that's
acceptable in my view- we shouldn't be pushing features which have known
issues that we then have to fight about how to fix it at the last minute
and with the constraint that we can't make any big changes.

Thanks,

Stephen

signature.asc
Description: PGP signature

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

On Mon, Aug 5, 2019 at 10:21 AM Stephen Frost  wrote:
> Just to be clear, I brought up this exact concern back in *November*:
>
> https://www.postgresql.org/message-id/20181127153405.GX3415%40tamriel.snowman.net
>
> And now because this was pushed forward and the concerns that I raised
> ignored, we're having to deal with this towards the end of the release
> cycle instead of during normal development.

I disagree. My analysis is that you're blocking a straightforward bug
fix because we're not prepared to redesign the world to match your
expectations. The actual point of controversy at the moment, as I
understand it, is this: if the backend, while rewriting
postgresql.auto.conf, discovers that it contains duplicates, should we
(a) give a WARNING or (b) not?

The argument for not doing that is pretty simple: if we give a WARNING
when this happens, then every tool that appends to
postgresql.auto.conf has to be responsible for making sure to remove
duplicates along the way.  To do that reliably, it needs a
client-accessible version of all the GUC parsing stuff.  You refer to
this above as an "assumption," but it seems to me that a more accurate
word would be "fact."  Now, I don't think anyone would disagree with
the idea that it possible to do it in an only-approximately-correct
way pretty easily: just match the first word of the line against the
GUC you're proposing to set, and drop the line if it matches. If you
want it to be exactly correct, though, you either need to run the
original code, or your own custom code that behaves in exactly the
same way.  And since the original code runs only in the server, it
follows directly that if you are not running inside the server, you
cannot be running the original code. How you can label any of that as
an "assumption" is beyond me.

Now, I agree that IF we were prepared to provide a standalone
config-editing tool that removes duplicates, THEN it would not be
crazy to emit a WARNING if we find a duplicate, because we could
reasonably tell people to just use that tool. However, such a tool is
not trivial to construct, as evidenced by the fact that, on this very
thread, Ian tried and Andres thought the result contained too much
code duplication. Moreover, we are past feature freeze, which is the
wrong time to add altogether new things to the release, even if we had
code that everybody liked. Furthermore, even if we had such a tool and
even if it had already been committed, I would still not be in favor
of the WARNING, because duplicate settings in postgresql.auto.conf are
harmless unless you include a truly insane number of them, and there
is no reason for anybody to ever do that.  In a way, I sorta hope
somebody does do that, because if I get a problem report from a user
that they put 10 million copies of their recovery settings in
postgresql.auto.conf and the server now starts up very slowly, I am
going to have a good private laugh, and then suggest that they maybe
not do that.

In general, I am sympathetic to the argument that we ought to do tight
integrity-checking on inputs: that's one of the things for which
PostgreSQL is known, and it's a strength of the project. In this case,
though, the cost-benefit trade-off seems very poor to me: it just
makes life complicated without really buying us anything. The whole
reason postgresql.conf is a text file in the first place instead of
being stored in the catalogs is because you might not be able to start
the server if it's not set right, and if you can't edit it without
being able to start the server, then you're stuck. Indeed, one of the
key arguments in getting ALTER SYSTEM accepted in the first place was
that, if you put dumb settings into postgresql.auto.conf and borked
your system so it wouldn't start, you could always use a text editor
to undo it. Given that history, any argument that postgresql.auto.conf
is somehow different and should be subjected to tighter integrity
constraints does not resonate with me. Its mission is to be a
machine-editable postgresql.conf, not to be some other kind of file
that plays by a different set of rules.

I really don't understand why you're fighting so hard about this. We
have a long history of being skeptical about WARNING messages. If, on
the one hand, they are super-important, they might still get ignored
because it could be an automated context where nobody will see it; and
if, on the other hand, they are not that important, then emitting them
is just clutter in the first place. The particular WARNING under
discussion here is one that would likely only fire long after the
fact, when it's far too late to do anything about it, and when, in all
probability, no real harm has resulted anyway.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

2019-08-05 Thread Andres Freund

Hi,

On 2019-08-05 12:43:24 -0400, Tom Lane wrote:
> Yeah, good point.  There are a lot of other cases where you really
> don't want system startup to happen, too.

Agreed.

> On the other hand, people have also opined that they want full error
> checking on the inserted values, and that seems out of reach with less
> than a full running system (mumble extensions mumble).

I think the error checking ought to be about as complete as the one we
do during a normal postmaster startup. Afaict that requires loading
shared_preload_library extensions, but does *not* require booting up far
enough to run GUC checks in a context with database access.

The one possible "extension" to that that I can see is that arguably we
might want to error out if DefineCustom*Variable() doesn't think the
value is valid for a shared_preload_library, rather than just WARNING
(i.e. refuse to start). We can't really do that for other libraries, but
for shared_preload_libraries it might make sense.  Although I suspect
the better approach would be to just generally convert that to an error,
rather than having some startup specific logic.

Greetings,

Andres Freund

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

Robert Haas  writes:
> On Mon, Aug 5, 2019 at 12:25 PM Tom Lane  wrote:
>> Perhaps as a future improvement, but it's not happening for v12,
>> at least not unless you accept "use ALTER SYSTEM in a standalone
>> backend" as a usable answer.

> I'm not sure that would even work for the cases at issue ... because
> we're talking about setting up recovery parameters, and wouldn't the
> server run recovery before it got around to do anything with ALTER
> SYSTEM?

Yeah, good point.  There are a lot of other cases where you really
don't want system startup to happen, too.  On the other hand,
people have also opined that they want full error checking on
the inserted values, and that seems out of reach with less than
a full running system (mumble extensions mumble).

In the end, I think I don't buy Stephen's argument that there should
be a one-size-fits-all answer.  It seems entirely reasonable that
we'll have more than one way to do it, because the constraints are
different depending on what use-case you think about.

regards, tom lane

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

On Mon, Aug 5, 2019 at 12:25 PM Tom Lane  wrote:
> Perhaps as a future improvement, but it's not happening for v12,
> at least not unless you accept "use ALTER SYSTEM in a standalone
> backend" as a usable answer.

I'm not sure that would even work for the cases at issue ... because
we're talking about setting up recovery parameters, and wouldn't the
server run recovery before it got around to do anything with ALTER
SYSTEM?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

David Fetter  writes:
> On Mon, Aug 05, 2019 at 03:52:07PM +0900, Ian Barwick wrote:
>> On 8/4/19 1:59 AM, Tom Lane wrote:
>>> I think we should just accept the facts on the ground, which are that
>>> some tools modify pg.auto.conf by appending to it

>> +1. It's just a text file...

> So are crontab and /etc/passwd, but there are gizmos that help make it
> difficult for people to write complete gobbledygook to those. Does it
> make sense to discuss tooling of that type?

Perhaps as a future improvement, but it's not happening for v12,
at least not unless you accept "use ALTER SYSTEM in a standalone
backend" as a usable answer.

regards, tom lane

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

2019-08-05 Thread David Fetter

On Mon, Aug 05, 2019 at 03:52:07PM +0900, Ian Barwick wrote:
> On 8/4/19 1:59 AM, Tom Lane wrote:> Tomas Vondra 
>  writes:
> >> On Fri, Aug 02, 2019 at 06:08:02PM -0700, Andres Freund wrote:
> >>> We're WAY WAY past feature freeze. This isn't the time to rewrite guc.c,
> >>> guc-file.l to be suitable for running outside of a backend environment.
> >
> >> Right. And even if we had the code, it's not quite backpatchable (which
> >> we probably should do, considering this is a general ALTER SYSTEM issue,
> >> so not pg12-only).
> >
> >> Not to mention there's no clear consensus this is actually desirable.
> >> I'd argue forcing external tools (written in arbitrary language) to use
> >> this library (written in C), just to modify a "stupid" text file is a
> >> bit overkill. IMO duplicates don't make the file invalid, we should
> >> handle that correctly/gracefully, so I don't see why external tools
> >> could not simply append to the file. We can deduplicate the file when
> >> starting the server, on ALTER SYSTEM, or some other time.
> >
> > Yup.  I'd also point out that even if we had a command-line tool of this
> > sort, there would be scenarios where it's not practical or not convenient
> > to use.  We need not go further than "my tool needs to work with existing
> > PG releases" to think of good examples.
> 
> I suspect this hasn't been an issue before, simply because until the removal
> of recovery.conf AFAIK there hasn't been a general use-case where you'd need
> to modify pg.auto.conf while the server is not running. The use-case which now
> exists (i.e. for writing replication configuration) is one where the tool will
> need to be version-aware anyway (like pg_basebackup is), so I don't see that 
> as
> a particular deal-breaker.
> 
> But...
> 
> > I think we should just accept the facts on the ground, which are that
> > some tools modify pg.auto.conf by appending to it
> 
> +1. It's just a text file...

So are crontab and /etc/passwd, but there are gizmos that help make it
difficult for people to write complete gobbledygook to those. Does it
make sense to discuss tooling of that type?

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

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

2019-08-05 Thread Christoph Berg

Re: Tomas Vondra 2019-08-03 <20190803124111.2aaddumd7url5wmq@development>
> If we really want to give external tools a sensible (and optional) API
> to access the file, a simple command-line tool seems much better. Say we
> have something like
> 
>   pg_config_file -f PATH --set KEY VALUE
>   pg_config_file -f PATH --get KEY

Fwiw, Debian has pg_conftool (based on the perl lib around
PgCommon.pm):

NAME
   pg_conftool - read and edit PostgreSQL cluster configuration files

SYNOPSIS
   pg_conftool [options] [version cluster] [configfile] command

DESCRIPTION
   pg_conftool allows to show and set parameters in PostgreSQL 
configuration files.

   If version cluster is omitted, it defaults to the default cluster (see 
user_clusters(5) and postgresqlrc(5)). If configfile is
   omitted, it defaults to postgresql.conf. configfile can also be a path, 
in which case version cluster is ignored.

OPTIONS
   -b, --boolean
   Format boolean value as on or off (not for "show all").

   -s, --short
   Show only the value (instead of key = value pair).

   -v, --verbose
   Verbose output.

   --help
   Print help.

COMMANDS
   show parameter|all
   Show a parameter, or all present in this config file.

   set parameter value
   Set or update a parameter.

   remove parameter
   Remove (comment out) a parameter from a config file.

   edit
   Open the config file in an editor.  Unless $EDITOR is set, vi is 
used.

SEE ALSO
   user_clusters(5), postgresqlrc(5)

AUTHOR
   Christoph Berg 

Debian2019-07-15
PG_CONFTOOL(1)

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

Isaac Morland  writes:
> Here's a radical suggestion: replace postgresql.auto.conf with a directory
> containing multiple files. Each file is named after a configuration
> parameter, and its content is the value of the parameter.

Hmm ... that seems like a lot of work and code churn --- in particular,
guaranteed breakage of code that works today --- to solve a problem
we haven't got.

The problem we do have is lack of documentation, which this wouldn't
in itself remedy.

> In order to prevent confusing and surprising behaviour, the system should
> complain vociferously if it finds a configuration parameter file that is
> not named after a defined parameter, rather than just ignoring it.

As has been pointed out repeatedly, the set of known parameters just
isn't that stable.  Different builds can recognize different sets of
GUCs, even without taking extensions into account.

regards, tom lane

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

Tomas Vondra  writes:
> On Mon, Aug 05, 2019 at 10:21:39AM -0400, Stephen Frost wrote:
>> I'd be happier with one set of code at least being the recommended
>> approach to modifying the file and only one set of code in our codebase
>> that's messing with .auto.conf, so that, hopefully, it's done
>> consistently and properly and in a way that everything agrees on and
>> expects, but if we can't get there due to concerns about where we are in
>> the release cycle, et al, then let's at least document what is
>> *supposed* to happen and have our code do so.

> I think fixing ALTER SYSTEM to handle duplicities, and documenting the
> basic rules about modifying .auto.conf is the way to go.

I agree.  So the problem at the moment is we lack a documentation
patch.  Who wants to write it?

regards, tom lane

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

2019-08-05 Thread Tomas Vondra

On Mon, Aug 05, 2019 at 10:21:39AM -0400, Stephen Frost wrote:

Greetings,

* Tomas Vondra (tomas.von...@2ndquadrant.com) wrote:

On Fri, Aug 02, 2019 at 06:38:46PM -0400, Stephen Frost wrote:
>On Fri, Aug 2, 2019 at 18:27 Tom Lane  wrote:
>>Tomas Vondra  writes:
>>> There seems to be a consensus that this this not a pg_basebackup issue
>>> (i.e. duplicate values don't make the file invalid), and it should be
>>> handled in ALTER SYSTEM.
>>
>>Yeah.  I doubt pg_basebackup is the only actor that can create such
>>situations.
>>
>>> The proposal seems to be to run through the .auto.conf file, remove any
>>> duplicates, and append the new entry at the end. That seems reasonable.
>>
>>+1
>
>I disagree that this should only be addressed in alter system, as I’ve said
>before and as others have agreed with.  Having one set of code that can be
>used to update parameters in the auto.conf and then have that be used by
>pg_basebackup, alter system, and external tools, is the right approach.
>
>The idea that alter system should be the only thing that doesn’t just
>append changes to the file is just going to lead to confusion and bugs down
>the road.

I don't remember any suggestions ALTER SYSTEM should be the only thing
that can rewrite the config file, but maybe it's buried somewhere in the
thread history. The current proposal certainly does not prohibit any
external tool from doing so, it just says we should expect duplicates.

There's an ongoing assumption that's been made that only ALTER SYSTEM
could make these changes because nothing else has the full GUC system
and a running PG instance to validate everything.

The suggestion that an external tool could do it goes against that.

If we can't, for whatever reason, work our way towards having code that
external tools could leverage to manage .auto.conf, then if we could at
least document what the expectations are and what tools can/can't do
with the file, that would put us in a better position than where we are
now.

IMO documenting the basic rules, and then doing some cleanup/validation
at instance start is the only practical solution, really.

You can't really validate "everything" without a running instance,
because that's the only place where you have GUCs defined by extensions.
I don't see how that could work for external tools, expected to run
exactly when the instance is not running.

I can't think of a use case where simply appending to the file would not
be perfectly sufficient. You can't really do much when the instance is
not running.

I strongly believe that whatever the rules and expectations are that we
come up with, both ALTER SYSTEM and the in-core and external tools
should follow them.

I'm not against giving external tools such capability, in whatever way
we think is appropriate (library, command-line binary, ...).

I'm against (a) making that a requirement for the external tools,
instead of just allowing them to append to the file, and (b) trying to
do that in PG12. We're at beta3, we don't even have any patch, and it
does quite work for past releases (although it's not that pressing
there, thanks to still having recovery.conf).

If we say to that tools should expect duplicates in the file, then
ALTER SYSTEM should as well, which was the whole issue in the first
place- ALTER SYSTEM didn't expect duplicates, but the external tools and
the GUC system did.

Sure.

If we say that it's acceptable for something to remove duplicate GUC
entries from the file, keeping the last one, then external tools should
feel comfortable doing that too and we should make it clear what
"duplicate" means here and how to identify one.

Sure. I don't see why the external tools would bother with doing that,
but I agree there's no reason not to document what duplicates mean.

If we say it's optional for a tool to remove duplicates, then we should
point out the risk of "running out of disk space" for tool authors to
consider.  I don't agree with the idea that tool authors should be asked
to depend on someone running ALTER SYSTEM to address that risk.  If
there's a strong feeling that tool authors should be able to depend on
PG to perform that cleanup for them, then we should use a mechanism to
do so which doesn't involve an entirely optional feature.

Considering the external tools are only allowed to modify the file while
the instance is not running, and that most instances are running all the
time, I very much doubt this is a risk we need to worry about.

And I don't see why we'd have to run ALTER SYSTEM - I proposed to do the
cleanup at instance start too.

For reference, all of the above, while not as cleanly as it could have
been, was addressed with the recovery.conf/recovery.done system.  Tool
authors had a good sense that they could replace that file, and that PG
would clean it up at exactly the right moment, and there wasn't this
ugly interaction with ALTER SYSTEM to have to worry about.  That none of
this was really even discussed or addressed

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

2019-08-05 Thread Isaac Morland

Here's a radical suggestion: replace postgresql.auto.conf with a directory
containing multiple files. Each file is named after a configuration
parameter, and its content is the value of the parameter.

So to remove a special configuration parameter, delete its file. To set it,
write the file, replacing an existing file if it exists.

For this to work unambiguously we would have to specify an exact,
case-sensitive, form of every parameter name that must be used within the
auto conf directory. I would suggest using the form listed in the
documentation (i.e., lower case, to my knowledge).

In order to prevent confusing and surprising behaviour, the system should
complain vociferously if it finds a configuration parameter file that is
not named after a defined parameter, rather than just ignoring it.

On Mon, 5 Aug 2019 at 10:21, Stephen Frost  wrote:

> Greetings,
>
> * Tomas Vondra (tomas.von...@2ndquadrant.com) wrote:
> > On Fri, Aug 02, 2019 at 06:38:46PM -0400, Stephen Frost wrote:
> > >On Fri, Aug 2, 2019 at 18:27 Tom Lane  wrote:
> > >>Tomas Vondra  writes:
> > >>> There seems to be a consensus that this this not a pg_basebackup
> issue
> > >>> (i.e. duplicate values don't make the file invalid), and it should be
> > >>> handled in ALTER SYSTEM.
> > >>
> > >>Yeah.  I doubt pg_basebackup is the only actor that can create such
> > >>situations.
> > >>
> > >>> The proposal seems to be to run through the .auto.conf file, remove
> any
> > >>> duplicates, and append the new entry at the end. That seems
> reasonable.
> > >>
> > >>+1
> > >
> > >I disagree that this should only be addressed in alter system, as I’ve
> said
> > >before and as others have agreed with.  Having one set of code that can
> be
> > >used to update parameters in the auto.conf and then have that be used by
> > >pg_basebackup, alter system, and external tools, is the right approach.
> > >
> > >The idea that alter system should be the only thing that doesn’t just
> > >append changes to the file is just going to lead to confusion and bugs
> down
> > >the road.
> >
> > I don't remember any suggestions ALTER SYSTEM should be the only thing
> > that can rewrite the config file, but maybe it's buried somewhere in the
> > thread history. The current proposal certainly does not prohibit any
> > external tool from doing so, it just says we should expect duplicates.
>
> There's an ongoing assumption that's been made that only ALTER SYSTEM
> could make these changes because nothing else has the full GUC system
> and a running PG instance to validate everything.
>
> The suggestion that an external tool could do it goes against that.
>
> If we can't, for whatever reason, work our way towards having code that
> external tools could leverage to manage .auto.conf, then if we could at
> least document what the expectations are and what tools can/can't do
> with the file, that would put us in a better position than where we are
> now.
>
> I strongly believe that whatever the rules and expectations are that we
> come up with, both ALTER SYSTEM and the in-core and external tools
> should follow them.
>
> If we say to that tools should expect duplicates in the file, then
> ALTER SYSTEM should as well, which was the whole issue in the first
> place- ALTER SYSTEM didn't expect duplicates, but the external tools and
> the GUC system did.
>
> If we say that it's acceptable for something to remove duplicate GUC
> entries from the file, keeping the last one, then external tools should
> feel comfortable doing that too and we should make it clear what
> "duplicate" means here and how to identify one.
>
> If we say it's optional for a tool to remove duplicates, then we should
> point out the risk of "running out of disk space" for tool authors to
> consider.  I don't agree with the idea that tool authors should be asked
> to depend on someone running ALTER SYSTEM to address that risk.  If
> there's a strong feeling that tool authors should be able to depend on
> PG to perform that cleanup for them, then we should use a mechanism to
> do so which doesn't involve an entirely optional feature.
>
> For reference, all of the above, while not as cleanly as it could have
> been, was addressed with the recovery.conf/recovery.done system.  Tool
> authors had a good sense that they could replace that file, and that PG
> would clean it up at exactly the right moment, and there wasn't this
> ugly interaction with ALTER SYSTEM to have to worry about.  That none of
> this was really even discussed or addressed previously even after being
> pointed out is really disappointing.
>
> Just to be clear, I brought up this exact concern back in *November*:
>
>
> https://www.postgresql.org/message-id/20181127153405.GX3415%40tamriel.snowman.net
>
> And now because this was pushed forward and the concerns that I raised
> ignored, we're having to deal with this towards the end of the release
> cycle instead of during normal development.  The things we're talking
> about now and which I'm

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

Greetings,

* Tomas Vondra (tomas.von...@2ndquadrant.com) wrote:
> On Fri, Aug 02, 2019 at 06:38:46PM -0400, Stephen Frost wrote:
> >On Fri, Aug 2, 2019 at 18:27 Tom Lane  wrote:
> >>Tomas Vondra  writes:
> >>> There seems to be a consensus that this this not a pg_basebackup issue
> >>> (i.e. duplicate values don't make the file invalid), and it should be
> >>> handled in ALTER SYSTEM.
> >>
> >>Yeah.  I doubt pg_basebackup is the only actor that can create such
> >>situations.
> >>
> >>> The proposal seems to be to run through the .auto.conf file, remove any
> >>> duplicates, and append the new entry at the end. That seems reasonable.
> >>
> >>+1
> >
> >I disagree that this should only be addressed in alter system, as I’ve said
> >before and as others have agreed with.  Having one set of code that can be
> >used to update parameters in the auto.conf and then have that be used by
> >pg_basebackup, alter system, and external tools, is the right approach.
> >
> >The idea that alter system should be the only thing that doesn’t just
> >append changes to the file is just going to lead to confusion and bugs down
> >the road.
> 
> I don't remember any suggestions ALTER SYSTEM should be the only thing
> that can rewrite the config file, but maybe it's buried somewhere in the
> thread history. The current proposal certainly does not prohibit any
> external tool from doing so, it just says we should expect duplicates.

There's an ongoing assumption that's been made that only ALTER SYSTEM
could make these changes because nothing else has the full GUC system
and a running PG instance to validate everything.

The suggestion that an external tool could do it goes against that.

If we can't, for whatever reason, work our way towards having code that
external tools could leverage to manage .auto.conf, then if we could at
least document what the expectations are and what tools can/can't do
with the file, that would put us in a better position than where we are
now.

I strongly believe that whatever the rules and expectations are that we
come up with, both ALTER SYSTEM and the in-core and external tools
should follow them.

If we say to that tools should expect duplicates in the file, then
ALTER SYSTEM should as well, which was the whole issue in the first
place- ALTER SYSTEM didn't expect duplicates, but the external tools and
the GUC system did.

If we say that it's acceptable for something to remove duplicate GUC
entries from the file, keeping the last one, then external tools should
feel comfortable doing that too and we should make it clear what
"duplicate" means here and how to identify one.

If we say it's optional for a tool to remove duplicates, then we should
point out the risk of "running out of disk space" for tool authors to
consider.  I don't agree with the idea that tool authors should be asked
to depend on someone running ALTER SYSTEM to address that risk.  If
there's a strong feeling that tool authors should be able to depend on
PG to perform that cleanup for them, then we should use a mechanism to
do so which doesn't involve an entirely optional feature.

For reference, all of the above, while not as cleanly as it could have
been, was addressed with the recovery.conf/recovery.done system.  Tool
authors had a good sense that they could replace that file, and that PG
would clean it up at exactly the right moment, and there wasn't this
ugly interaction with ALTER SYSTEM to have to worry about.  That none of
this was really even discussed or addressed previously even after being
pointed out is really disappointing.

Just to be clear, I brought up this exact concern back in *November*:

https://www.postgresql.org/message-id/20181127153405.GX3415%40tamriel.snowman.net

And now because this was pushed forward and the concerns that I raised
ignored, we're having to deal with this towards the end of the release
cycle instead of during normal development.  The things we're talking
about now and which I'm getting push-back on because of the release
cycle situation were specifically suggestions I made in the above email
in November where I also brought up concern that ALTER SYSTEM would be
confused by the duplicates- giving external tools guideance on how to
modify .auto.conf, or providing them a tool (or library), or both.

None of this should be coming as a surprise to anyone who was following
and I feel we should be upset that this was left to such a late point in
the release cycle to address these issues.

> >>There was a discussion whether to print warnings about the duplicates. I
> >>> personally see not much point in doing that - if we consider duplicates
> >>> to be expected, and if ALTER SYSTEM has the license to rework the config
> >>> file any way it wants, why warn about it?
> >>
> >>Personally I agree that warnings are unnecessary.
> >
> >And at least Magnus and I disagree with that, as I recall from this
> >thread.  Let’s have a clean and clear way to modify the auto.conf and have
> >everything that

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


On 8/4/19 1:59 AM, Tom Lane wrote:> Tomas Vondra  
writes:
>> On Fri, Aug 02, 2019 at 06:08:02PM -0700, Andres Freund wrote:
>>> We're WAY WAY past feature freeze. This isn't the time to rewrite guc.c,
>>> guc-file.l to be suitable for running outside of a backend environment.
>
>> Right. And even if we had the code, it's not quite backpatchable (which
>> we probably should do, considering this is a general ALTER SYSTEM issue,
>> so not pg12-only).
>
>> Not to mention there's no clear consensus this is actually desirable.
>> I'd argue forcing external tools (written in arbitrary language) to use
>> this library (written in C), just to modify a "stupid" text file is a
>> bit overkill. IMO duplicates don't make the file invalid, we should
>> handle that correctly/gracefully, so I don't see why external tools
>> could not simply append to the file. We can deduplicate the file when
>> starting the server, on ALTER SYSTEM, or some other time.
>
> Yup.  I'd also point out that even if we had a command-line tool of this
> sort, there would be scenarios where it's not practical or not convenient
> to use.  We need not go further than "my tool needs to work with existing
> PG releases" to think of good examples.

I suspect this hasn't been an issue before, simply because until the removal
of recovery.conf AFAIK there hasn't been a general use-case where you'd need
to modify pg.auto.conf while the server is not running. The use-case which now
exists (i.e. for writing replication configuration) is one where the tool will
need to be version-aware anyway (like pg_basebackup is), so I don't see that as
a particular deal-breaker.

But...

> I think we should just accept the facts on the ground, which are that
> some tools modify pg.auto.conf by appending to it

+1. It's just a text file...

> and say that that's supported as long as the file doesn't get unreasonably 
long.

Albeit with the caveat that the server should not be running.

Not sure how you define "unreasonably long" though.

> I'm not at all on board with inventing a requirement for pg.auto.conf
> to track its modification history.  I don't buy that that's a
> widespread need in the first place; if I did buy it, that file
> itself is not where to keep the history; and in any case, it'd be
> a new feature and it's way too late for v12.

Yeah, that's way outside of the scope of this issue.


Regards

Ian Barwick

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

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


On 8/4/19 4:13 AM, Tom Lane wrote:

Ian Barwick  writes:

On 8/3/19 7:27 AM, Tom Lane wrote:

Tomas Vondra  writes:

The main issue however is that no code was written yet.



Seems like it ought to be relatively simple ... but I didn't look.



The patch I originally sent does exactly this.


Ah, you did send a patch, but that tries to maintain the existing behavior
of replacing the last occurrence in-place.  I think it's simpler and more
sensible to just make a sweep to delete all matches, and then append the
new setting (if any) at the end, as attached.


Yes, that is less convoluted.


A more aggressive patch would try to de-duplicate the entire list, not
just the current target entry ... but I'm not really excited about doing
that in a back-patchable bug fix.


I thought about doing that but it's more of a nice-to-have and not essential
to fix the issue, as any other duplicate entries will get removed the next
time ALTER SYSTEM is run on the entry in question. Maybe as part of a future
improvement.


I looked at the TAP test you proposed and couldn't quite convince myself
that it was worth the trouble.  A new test within an existing suite
would likely be fine, but a whole new src/test/ subdirectory just for
pg.auto.conf seems a bit much.  (Note that the buildfarm and possibly
the MSVC scripts would have to be taught about each such subdirectory.)


Didn't know that, but couldn't find anywhere obvious to put the test.


At the same time, we lack any better place to put such a test :-(.
Maybe it's time for a "miscellaneous TAP tests" subdirectory?


Sounds reasonable.


Regards

Ian Barwick



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

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

2019-08-03 Thread Tom Lane

Ian Barwick  writes:
> On 8/3/19 7:27 AM, Tom Lane wrote:
>> Tomas Vondra  writes:
>>> The main issue however is that no code was written yet.

>> Seems like it ought to be relatively simple ... but I didn't look.

> The patch I originally sent does exactly this.

Ah, you did send a patch, but that tries to maintain the existing behavior
of replacing the last occurrence in-place.  I think it's simpler and more
sensible to just make a sweep to delete all matches, and then append the
new setting (if any) at the end, as attached.

A more aggressive patch would try to de-duplicate the entire list, not
just the current target entry ... but I'm not really excited about doing
that in a back-patchable bug fix.

I looked at the TAP test you proposed and couldn't quite convince myself
that it was worth the trouble.  A new test within an existing suite
would likely be fine, but a whole new src/test/ subdirectory just for
pg.auto.conf seems a bit much.  (Note that the buildfarm and possibly
the MSVC scripts would have to be taught about each such subdirectory.)
At the same time, we lack any better place to put such a test :-(.
Maybe it's time for a "miscellaneous TAP tests" subdirectory?

regards, tom lane

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index fc46360..12abbb2 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -7850,40 +7850,37 @@ replace_auto_config_value(ConfigVariable **head_p, ConfigVariable **tail_p,
 		  const char *name, const char *value)
 {
 	ConfigVariable *item,
+			   *next,
 			   *prev = NULL;

-	/* Search the list for an existing match (we assume there's only one) */
-	for (item = *head_p; item != NULL; item = item->next)
+	/*
+	 * Remove any existing match(es) for "name".  Normally there'd be at most
+	 * one, but if external tools have modified the config file, there could
+	 * be more.
+	 */
+	for (item = *head_p; item != NULL; item = next)
 	{
+		next = item->next;
 		if (strcmp(item->name, name) == 0)
 		{
-			/* found a match, replace it */
-			pfree(item->value);
-			if (value != NULL)
-			{
-/* update the parameter value */
-item->value = pstrdup(value);
-			}
+			/* found a match, delete it */
+			if (prev)
+prev->next = next;
 			else
-			{
-/* delete the configuration parameter from list */
-if (*head_p == item)
-	*head_p = item->next;
-else
-	prev->next = item->next;
-if (*tail_p == item)
-	*tail_p = prev;
+*head_p = next;
+			if (next == NULL)
+*tail_p = prev;

-pfree(item->name);
-pfree(item->filename);
-pfree(item);
-			}
-			return;
+			pfree(item->value);
+			pfree(item->name);
+			pfree(item->filename);
+			pfree(item);
 		}
-		prev = item;
+		else
+			prev = item;
 	}

-	/* Not there; no work if we're trying to delete it */
+	/* Done if we're trying to delete it */
 	if (value == NULL)
 		return;

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

2019-08-03 Thread Tom Lane

Tomas Vondra  writes:
> On Fri, Aug 02, 2019 at 06:08:02PM -0700, Andres Freund wrote:
>> We're WAY WAY past feature freeze. This isn't the time to rewrite guc.c,
>> guc-file.l to be suitable for running outside of a backend environment.

> Right. And even if we had the code, it's not quite backpatchable (which
> we probably should do, considering this is a general ALTER SYSTEM issue,
> so not pg12-only).

> Not to mention there's no clear consensus this is actually desirable.
> I'd argue forcing external tools (written in arbitrary language) to use
> this library (written in C), just to modify a "stupid" text file is a
> bit overkill. IMO duplicates don't make the file invalid, we should
> handle that correctly/gracefully, so I don't see why external tools
> could not simply append to the file. We can deduplicate the file when
> starting the server, on ALTER SYSTEM, or some other time.

Yup.  I'd also point out that even if we had a command-line tool of this
sort, there would be scenarios where it's not practical or not convenient
to use.  We need not go further than "my tool needs to work with existing
PG releases" to think of good examples.

I think we should just accept the facts on the ground, which are that
some tools modify pg.auto.conf by appending to it, and say that that's
supported as long as the file doesn't get unreasonably long.

I'm not at all on board with inventing a requirement for pg.auto.conf
to track its modification history.  I don't buy that that's a
widespread need in the first place; if I did buy it, that file
itself is not where to keep the history; and in any case, it'd be
a new feature and it's way too late for v12.

regards, tom lane

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

2019-08-03 Thread Tomas Vondra

On Fri, Aug 02, 2019 at 06:08:02PM -0700, Andres Freund wrote:

Hi,

On 2019-08-02 20:57:20 -0400, Stephen Frost wrote:

No, I’m saying that we already *have* a library and we can add a few
functions to it and if people want to leverage those functions then they
can write glue code to do so, just like was done with libpq. The argument
that “we shouldn’t put code into the common library because only tools
written in C can use the common library” is what I was specifically taking
exception with and your response doesn’t change my opinion of that argument
one bit.

Wait, which library is this? And which code is suitable for being put in
a library right now?

We're WAY WAY past feature freeze. This isn't the time to rewrite guc.c,
guc-file.l to be suitable for running outside of a backend environment.

Right. And even if we had the code, it's not quite backpatchable (which
we probably should do, considering this is a general ALTER SYSTEM issue,
so not pg12-only).

Not to mention there's no clear consensus this is actually desirable.
I'd argue forcing external tools (written in arbitrary language) to use
this library (written in C), just to modify a "stupid" text file is a
bit overkill. IMO duplicates don't make the file invalid, we should
handle that correctly/gracefully, so I don't see why external tools
could not simply append to the file. We can deduplicate the file when
starting the server, on ALTER SYSTEM, or some other time.

If we really want to give external tools a sensible (and optional) API
to access the file, a simple command-line tool seems much better. Say we
have something like

  pg_config_file -f PATH --set KEY VALUE
  pg_config_file -f PATH --get KEY

to set / query value of an option. I still don't see why we should force
people to use that (instead of appending to the file), though. Not to
mention it's way of out pg12 scope.

Apparently I don’t have the experiences that you do as I’ve not seen a lot
of systems which are constantly rewriting the conf file to the point where
keeping the versions would be likely to add up to anything interesting.

Shrug. I've e.g. seen people continuously (every few minutes or so)
change autovacuum settings depending on load and observed response
times. Which isn't even a crazy thing to do.

I agree a history of the config values is useful in some cases, but I
very much doubt stashing them in the config file is sensible. It gives
you pretty much no metadata (like timestamp of the change), certainly
not in an easy-to-query way. I've seen people storing that info in a
monitoring system (so a timeseries for each autovacuum setting), or we
might add a hook to ALTER SYSTEM so that we could feed it somewhere.

But I see little evidence stashing the changes in a file indefinitely is
a good idea, especially when there's no way to clear old data etc. It
seems more like a rather artificial use case invented to support the
idea of keeping the duplicates.

Designing the system around “well, we don’t think you’ll modify the file
very much from an external tool, so we just won’t worry about it, but if
you use alter system then we will clean things up” certainly doesn’t strike
me as terribly principled.

Well. You shouldn't change postgresql.conf.auto while the server is
running, for fairly obvious reasons. Therefore external tools not using
ALTER SYSTEM only make sense when the server is not running. And I don't
think it's a crazy to assume that PG servers where you'd regularly
change the config are running most of the time.

Right.

And again, we're talking about v12 here. I don't think anybody is
arguing that we shouldn't provide library/commandline tools to make make
changes to postgresql.auto.conf conveniently possible without
duplicating lines. BUT not for v12, especially not because as the person
arguing for this, you've not provided a patch providing such a library.

+1 million here

> Personally, I don’t buy the “run out of disk space” argument but if we are
> > going to go there then we should apply it appropriately.
> >
> > Having the history of changes to auto.conf would actually be quite
> useful,
> > imv, and worth a bit of disk space (heck, it’s not exactly uncommon for
> > people to keep their config files in git repos..). I’d suggest we also
> > include the date/time of when the modification was made.
>
> That just seems like an entirely different project. It seems blindlingly
> obvious that we can't keep the entire history in the file that we're
> going to be parsing on a regular basis. Having some form of config
> history tracking might be interesting, but I think it's utterly and
> completely independent from what we need to fix for v12.

We don’t parse the file on anything like a “regular” basis.

Well, everytime somebody does pg_reload_conf(), which for systems that
do frequent ALTER SYSTEMs, is kinda frequent too...

Right.

regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7

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

Hi,

On 2019-08-02 20:57:20 -0400, Stephen Frost wrote:
> No, I’m saying that we already *have* a library and we can add a few
> functions to it and if people want to leverage those functions then they
> can write glue code to do so, just like was done with libpq. The argument
> that “we shouldn’t put code into the common library because only tools
> written in C can use the common library” is what I was specifically taking
> exception with and your response doesn’t change my opinion of that argument
> one bit.

Wait, which library is this? And which code is suitable for being put in
a library right now?

We're WAY WAY past feature freeze. This isn't the time to rewrite guc.c,
guc-file.l to be suitable for running outside of a backend environment.

> Apparently I don’t have the experiences that you do as I’ve not seen a lot
> of systems which are constantly rewriting the conf file to the point where
> keeping the versions would be likely to add up to anything interesting.

Shrug. I've e.g. seen people continuously (every few minutes or so)
change autovacuum settings depending on load and observed response
times. Which isn't even a crazy thing to do.

> Designing the system around “well, we don’t think you’ll modify the file
> very much from an external tool, so we just won’t worry about it, but if
> you use alter system then we will clean things up” certainly doesn’t strike
> me as terribly principled.

Well. You shouldn't change postgresql.conf.auto while the server is
running, for fairly obvious reasons. Therefore external tools not using
ALTER SYSTEM only make sense when the server is not running. And I don't
think it's a crazy to assume that PG servers where you'd regularly
change the config are running most of the time.

And again, we're talking about v12 here. I don't think anybody is
arguing that we shouldn't provide library/commandline tools to make make
changes to postgresql.auto.conf conveniently possible without
duplicating lines. BUT not for v12, especially not because as the person
arguing for this, you've not provided a patch providing such a library.

> > Personally, I don’t buy the “run out of disk space” argument but if we are
> > > going to go there then we should apply it appropriately.
> > >
> > > Having the history of changes to auto.conf would actually be quite
> > useful,
> > > imv, and worth a bit of disk space (heck, it’s not exactly uncommon for
> > > people to keep their config files in git repos..). I’d suggest we also
> > > include the date/time of when the modification was made.
> >
> > That just seems like an entirely different project. It seems blindlingly
> > obvious that we can't keep the entire history in the file that we're
> > going to be parsing on a regular basis. Having some form of config
> > history tracking might be interesting, but I think it's utterly and
> > completely independent from what we need to fix for v12.

> We don’t parse the file on anything like a “regular” basis.

Well, everytime somebody does pg_reload_conf(), which for systems that
do frequent ALTER SYSTEMs, is kinda frequent too...

Greetings,

Andres Freund

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

Greetings,

On Fri, Aug 2, 2019 at 20:46 Andres Freund  wrote:

> Hi,
>
> On 2019-08-02 20:27:25 -0400, Stephen Frost wrote:
> > On Fri, Aug 2, 2019 at 18:47 Tom Lane  wrote:
> > > Stephen Frost  writes:
> > > > On Fri, Aug 2, 2019 at 18:27 Tom Lane  wrote:
> > > >>> The proposal seems to be to run through the .auto.conf file,
> remove any
> > > >>> duplicates, and append the new entry at the end. That seems
> reasonable.
> > >
> > > >> +1
> > >
> > > > I disagree that this should only be addressed in alter system, as
> I’ve
> > > said
> > > > before and as others have agreed with.  Having one set of code that
> can
> > > be
> > > > used to update parameters in the auto.conf and then have that be
> used by
> > > > pg_basebackup, alter system, and external tools, is the right
> approach.
> > >
> > > I don't find that to be necessary or even desirable.  Many (most?) of
> the
> > > situations where this would be important wouldn't have access to a
> running
> > > backend, and maybe not to any PG code at all --- what if your tool
> isn't
> > > written in C?
> >
> >
> > What if you want to access PG and your tool isn’t written in C?  You use
> a
> > module, extension, package, whatever, that provides the glue between what
> > your language wants and what the C library provides.  There’s psycopg2
> for
> > python, DBD::Pg for Perl, et al, and they use libpq. There’s languages
> that
> > like to write their own too, like the JDBC driver, the Golang driver, but
> > that doesn’t mean we shouldn’t provide libpq or that non-C tools can’t
> > leverage libpq.  This argument is just not sensible.
>
> Oh, comeon. Are you seriously suggesting that a few commands to add a a
> new config setting to postgresql.auto.conf will cause a lot of people to
> write wrappers around $new_config_library in their language of choice,
> because they did the same for libpq? And that we should design such a
> library, for v12?


No, I’m saying that we already *have* a library and we can add a few
functions to it and if people want to leverage those functions then they
can write glue code to do so, just like was done with libpq. The argument
that “we shouldn’t put code into the common library because only tools
written in C can use the common library” is what I was specifically taking
exception with and your response doesn’t change my opinion of that argument
one bit.

> I think it's perfectly fine to say that external tools need only append
> > > to the file, which will require no special tooling.  But then we need
> > > ALTER SYSTEM to be willing to clean out duplicates, if only so you
> don't
> > > run out of disk space after awhile.
>
> > Uh, if you don’t ever run ALTER SYSTEM then you could also “run out of
> disk
> > space” due to external tools modifying by just adding to the file.
>
> That was commented upon in the emails you're replying to? It seems
> hardly likely that you'd get enough config entries to make that
> problematic while postgres is not running. While running it's a
> different story.


Apparently I don’t have the experiences that you do as I’ve not seen a lot
of systems which are constantly rewriting the conf file to the point where
keeping the versions would be likely to add up to anything interesting.

Designing the system around “well, we don’t think you’ll modify the file
very much from an external tool, so we just won’t worry about it, but if
you use alter system then we will clean things up” certainly doesn’t strike
me as terribly principled.

> Personally, I don’t buy the “run out of disk space” argument but if we are
> > going to go there then we should apply it appropriately.
> >
> > Having the history of changes to auto.conf would actually be quite
> useful,
> > imv, and worth a bit of disk space (heck, it’s not exactly uncommon for
> > people to keep their config files in git repos..). I’d suggest we also
> > include the date/time of when the modification was made.
>
> That just seems like an entirely different project. It seems blindlingly
> obvious that we can't keep the entire history in the file that we're
> going to be parsing on a regular basis. Having some form of config
> history tracking might be interesting, but I think it's utterly and
> completely independent from what we need to fix for v12.


We don’t parse the file on anything like a “regular” basis.

Thanks,

Stephen

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

Hi,

On 2019-08-02 20:27:25 -0400, Stephen Frost wrote:
> On Fri, Aug 2, 2019 at 18:47 Tom Lane  wrote:
> > Stephen Frost  writes:
> > > On Fri, Aug 2, 2019 at 18:27 Tom Lane  wrote:
> > >>> The proposal seems to be to run through the .auto.conf file, remove any
> > >>> duplicates, and append the new entry at the end. That seems reasonable.
> >
> > >> +1
> >
> > > I disagree that this should only be addressed in alter system, as I’ve
> > said
> > > before and as others have agreed with.  Having one set of code that can
> > be
> > > used to update parameters in the auto.conf and then have that be used by
> > > pg_basebackup, alter system, and external tools, is the right approach.
> >
> > I don't find that to be necessary or even desirable.  Many (most?) of the
> > situations where this would be important wouldn't have access to a running
> > backend, and maybe not to any PG code at all --- what if your tool isn't
> > written in C?
>
>
> What if you want to access PG and your tool isn’t written in C?  You use a
> module, extension, package, whatever, that provides the glue between what
> your language wants and what the C library provides.  There’s psycopg2 for
> python, DBD::Pg for Perl, et al, and they use libpq. There’s languages that
> like to write their own too, like the JDBC driver, the Golang driver, but
> that doesn’t mean we shouldn’t provide libpq or that non-C tools can’t
> leverage libpq.  This argument is just not sensible.

Oh, comeon. Are you seriously suggesting that a few commands to add a a
new config setting to postgresql.auto.conf will cause a lot of people to
write wrappers around $new_config_library in their language of choice,
because they did the same for libpq? And that we should design such a
library, for v12?

> I think it's perfectly fine to say that external tools need only append
> > to the file, which will require no special tooling.  But then we need
> > ALTER SYSTEM to be willing to clean out duplicates, if only so you don't
> > run out of disk space after awhile.

> Uh, if you don’t ever run ALTER SYSTEM then you could also “run out of disk
> space” due to external tools modifying by just adding to the file.

That was commented upon in the emails you're replying to? It seems
hardly likely that you'd get enough config entries to make that
problematic while postgres is not running. While running it's a
different story.

> Personally, I don’t buy the “run out of disk space” argument but if we are
> going to go there then we should apply it appropriately.
>
> Having the history of changes to auto.conf would actually be quite useful,
> imv, and worth a bit of disk space (heck, it’s not exactly uncommon for
> people to keep their config files in git repos..). I’d suggest we also
> include the date/time of when the modification was made.

That just seems like an entirely different project. It seems blindlingly
obvious that we can't keep the entire history in the file that we're
going to be parsing on a regular basis. Having some form of config
history tracking might be interesting, but I think it's utterly and
completely independent from what we need to fix for v12.

It seems pretty clear that there's more people disagreeing with your
position than agreeing with you. Because of this conflict there's not
been progress on this for weeks. I think it's beyond time that we just
do the minimal thing for v12, and then continue from there in v13.

- Andres

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

Greetings,

On Fri, Aug 2, 2019 at 18:47 Tom Lane  wrote:

> Stephen Frost  writes:
> > On Fri, Aug 2, 2019 at 18:27 Tom Lane  wrote:
> >>> The proposal seems to be to run through the .auto.conf file, remove any
> >>> duplicates, and append the new entry at the end. That seems reasonable.
>
> >> +1
>
> > I disagree that this should only be addressed in alter system, as I’ve
> said
> > before and as others have agreed with.  Having one set of code that can
> be
> > used to update parameters in the auto.conf and then have that be used by
> > pg_basebackup, alter system, and external tools, is the right approach.
>
> I don't find that to be necessary or even desirable.  Many (most?) of the
> situations where this would be important wouldn't have access to a running
> backend, and maybe not to any PG code at all --- what if your tool isn't
> written in C?

What if you want to access PG and your tool isn’t written in C?  You use a
module, extension, package, whatever, that provides the glue between what
your language wants and what the C library provides.  There’s psycopg2 for
python, DBD::Pg for Perl, et al, and they use libpq. There’s languages that
like to write their own too, like the JDBC driver, the Golang driver, but
that doesn’t mean we shouldn’t provide libpq or that non-C tools can’t
leverage libpq.  This argument is just not sensible.

I agree entirely that we want to be able to modify auto.conf without having
PG running (and without using single mode, bleh, that’s horrid..).  I think
we can accept that there we can’t necessarily *validate* that every option
is acceptable but that’s not the same as being able to simply parse the
file and modify a value.

I think it's perfectly fine to say that external tools need only append
> to the file, which will require no special tooling.  But then we need
> ALTER SYSTEM to be willing to clean out duplicates, if only so you don't
> run out of disk space after awhile.

Uh, if you don’t ever run ALTER SYSTEM then you could also “run out of disk
space” due to external tools modifying by just adding to the file.

Personally, I don’t buy the “run out of disk space” argument but if we are
going to go there then we should apply it appropriately.

Having the history of changes to auto.conf would actually be quite useful,
imv, and worth a bit of disk space (heck, it’s not exactly uncommon for
people to keep their config files in git repos..). I’d suggest we also
include the date/time of when the modification was made.

Thanks,

Stephen

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

Greetings,

On Fri, Aug 2, 2019 at 19:36 Ian Barwick 
wrote:

> On 8/3/19 8:24 AM, Andres Freund wrote:
> > Hi,
> >
> > On 2019-08-03 08:22:29 +0900, Ian Barwick wrote:
> >> What I came up with shoehorned a stripped-down version of the backend
> >> config parser into fe_utils and provides a function to modify
> pg.auto.conf
> >> in much the same way ALTER SYSTEM does, but with only the basic syntax
> >> checking provided by the parser of course. And for completeness a
> >> client utility which can be called by scripts etc.
> >
> >> I can clean it up and submit it later for reference (got distracted by
> other things
> >> recently) though I don't think it's a particularly good solution due to
> the
> >> lack of actual checks for the provided GUCSs (and the implementation
> >> is ugly anyway); something like what Andres suggests below would be far
> better.
> >
> > I think my main problem with that is that it duplicates a nontrivial
> > amount of code.
>
> That is indeed part of the ugliness of the implementation.


I agree that duplicate code isn’t good- the goal would be to eliminate the
duplication by having it be common code instead of duplicated.  We have
other code that’s common to the frontend and backend and I don’t doubt that
we will have more going forward...

Thanks,

Stephen

>

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


On 8/3/19 8:24 AM, Andres Freund wrote:

Hi,

On 2019-08-03 08:22:29 +0900, Ian Barwick wrote:

What I came up with shoehorned a stripped-down version of the backend
config parser into fe_utils and provides a function to modify pg.auto.conf
in much the same way ALTER SYSTEM does, but with only the basic syntax
checking provided by the parser of course. And for completeness a
client utility which can be called by scripts etc.



I can clean it up and submit it later for reference (got distracted by other 
things
recently) though I don't think it's a particularly good solution due to the
lack of actual checks for the provided GUCSs (and the implementation
is ugly anyway); something like what Andres suggests below would be far better.


I think my main problem with that is that it duplicates a nontrivial
amount of code.


That is indeed part of the ugliness of the implementation.


Regards

Ian Barwick

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

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

Hi,

On 2019-08-03 08:22:29 +0900, Ian Barwick wrote:
> What I came up with shoehorned a stripped-down version of the backend
> config parser into fe_utils and provides a function to modify pg.auto.conf
> in much the same way ALTER SYSTEM does, but with only the basic syntax
> checking provided by the parser of course. And for completeness a
> client utility which can be called by scripts etc.

> I can clean it up and submit it later for reference (got distracted by other 
> things
> recently) though I don't think it's a particularly good solution due to the
> lack of actual checks for the provided GUCSs (and the implementation
> is ugly anyway); something like what Andres suggests below would be far 
> better.

I think my main problem with that is that it duplicates a nontrivial
amount of code.

Greetings,

Andres Freund

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


On 8/3/19 7:56 AM, Andres Freund wrote:

Hi,

On 2019-08-02 18:47:07 -0400, Tom Lane wrote:

Stephen Frost  writes:

I disagree that this should only be addressed in alter system, as I’ve said
before and as others have agreed with.  Having one set of code that can be
used to update parameters in the auto.conf and then have that be used by
pg_basebackup, alter system, and external tools, is the right approach.


I don't find that to be necessary or even desirable.  Many (most?) of the
situations where this would be important wouldn't have access to a running
backend, and maybe not to any PG code at all --- what if your tool isn't
written in C?


I think a commandline tool to perform the equivalent of ALTER SYSTEM on
a shutdown cluster would be a great tool. It's easy enough to add
something with broken syntax, and further down the road such a tool
could not only ensure the syntax is correct, but also validate
individual settings as much as possible (obviously there's some hairy
issues here).


What I came up with shoehorned a stripped-down version of the backend
config parser into fe_utils and provides a function to modify pg.auto.conf
in much the same way ALTER SYSTEM does, but with only the basic syntax
checking provided by the parser of course. And for completeness a
client utility which can be called by scripts etc.

I can clean it up and submit it later for reference (got distracted by other 
things
recently) though I don't think it's a particularly good solution due to the
lack of actual checks for the provided GUCSs (and the implementation
is ugly anyway); something like what Andres suggests below would be far better.


Quite possibly the most realistic way to implement something like that
would be a postgres commandline switch, which'd start up far enough to
perform GUC checks and execute AlterSystem(), and then shut down
again. We already have -C, I think such an option could reasonably be
implemented alongside it.

Obviously this is widely out of scope for v12.



Regards


Ian Barwick


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

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


On 8/3/19 8:09 AM, Tom Lane wrote:

I wrote:

Andres Freund  writes:

I think a commandline tool to perform the equivalent of ALTER SYSTEM on
a shutdown cluster would be a great tool.



Perhaps, but ...



Obviously this is widely out of scope for v12.



... this.


Although, there's always

echo "alter system set work_mem = 4242;" | postgres --single

Maybe we could recommend that to tools that need to do
potentially-repetitive modifications?


The slight problem with that, particularly with the use-case
I am concerned with (writing replication configuration), is:

[2019-08-03 08:14:21 JST]FATAL:  0A000: standby mode is not supported 
by single-user servers

(I may be missing something obvious of course)


Regards

Ian Barwick

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

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

I wrote:
> Andres Freund  writes:
>> I think a commandline tool to perform the equivalent of ALTER SYSTEM on
>> a shutdown cluster would be a great tool.

> Perhaps, but ...

>> Obviously this is widely out of scope for v12.

> ... this.

Although, there's always

echo "alter system set work_mem = 4242;" | postgres --single

Maybe we could recommend that to tools that need to do
potentially-repetitive modifications?

regards, tom lane

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

Andres Freund  writes:
> On 2019-08-02 18:47:07 -0400, Tom Lane wrote:
>> I don't find that to be necessary or even desirable.  Many (most?) of the
>> situations where this would be important wouldn't have access to a running
>> backend, and maybe not to any PG code at all --- what if your tool isn't
>> written in C?

> I think a commandline tool to perform the equivalent of ALTER SYSTEM on
> a shutdown cluster would be a great tool.

Perhaps, but ...

> Obviously this is widely out of scope for v12.

... this.  It's entirely insane to think we're going to produce any such
thing for v12 (much less back-patch it into prior versions).  In the short
term I don't think there's any workable alternative except to decree that
"just append to the end" is a supported way to alter pg.auto.conf.

But, as you said, it's also not sane for ALTER SYSTEM to behave that way,
because it won't cope for long with repetitive modifications.  I think
we can get away with the "just append" recommendation for most external
drivers because they won't be doing that.  If they are, they'll need to
be smarter, and maybe some command-line tool would make their lives
simpler down the line.  But we aren't providing that in this cycle.

regards, tom lane

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

2019-08-02 Thread Tomas Vondra

On Fri, Aug 02, 2019 at 06:38:46PM -0400, Stephen Frost wrote:

Greetings,

On Fri, Aug 2, 2019 at 18:27 Tom Lane  wrote:

Tomas Vondra  writes:
> There seems to be a consensus that this this not a pg_basebackup issue
> (i.e. duplicate values don't make the file invalid), and it should be
> handled in ALTER SYSTEM.

Yeah.  I doubt pg_basebackup is the only actor that can create such
situations.

> The proposal seems to be to run through the .auto.conf file, remove any
> duplicates, and append the new entry at the end. That seems reasonable.

+1

I disagree that this should only be addressed in alter system, as I’ve said
before and as others have agreed with.  Having one set of code that can be
used to update parameters in the auto.conf and then have that be used by
pg_basebackup, alter system, and external tools, is the right approach.

The idea that alter system should be the only thing that doesn’t just
append changes to the file is just going to lead to confusion and bugs down
the road.

I don't remember any suggestions ALTER SYSTEM should be the only thing
that can rewrite the config file, but maybe it's buried somewhere in the
thread history. The current proposal certainly does not prohibit any
external tool from doing so, it just says we should expect duplicates.

As I said before, an alternative could be to make alter system simply
always append and declare that to be the way to update parameters in the
auto.conf.

That just seems strange, TBH.

There was a discussion whether to print warnings about the duplicates. I
> personally see not much point in doing that - if we consider duplicates
> to be expected, and if ALTER SYSTEM has the license to rework the config
> file any way it wants, why warn about it?

Personally I agree that warnings are unnecessary.

And at least Magnus and I disagree with that, as I recall from this
thread.  Let’s have a clean and clear way to modify the auto.conf and have
everything that touches the file update it in a consistent way.

Well, I personally don't feel very strongly about it. I think the
warnings will be a nuisance bothering people with expeced stuff, but I'm
not willing to fight against it.

regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

Hi,

On 2019-08-02 18:47:07 -0400, Tom Lane wrote:
> Stephen Frost  writes:
> > I disagree that this should only be addressed in alter system, as I’ve said
> > before and as others have agreed with.  Having one set of code that can be
> > used to update parameters in the auto.conf and then have that be used by
> > pg_basebackup, alter system, and external tools, is the right approach.
> 
> I don't find that to be necessary or even desirable.  Many (most?) of the
> situations where this would be important wouldn't have access to a running
> backend, and maybe not to any PG code at all --- what if your tool isn't
> written in C?

I think a commandline tool to perform the equivalent of ALTER SYSTEM on
a shutdown cluster would be a great tool. It's easy enough to add
something with broken syntax, and further down the road such a tool
could not only ensure the syntax is correct, but also validate
individual settings as much as possible (obviously there's some hairy
issues here).

Quite possibly the most realistic way to implement something like that
would be a postgres commandline switch, which'd start up far enough to
perform GUC checks and execute AlterSystem(), and then shut down
again. We already have -C, I think such an option could reasonably be
implemented alongside it.

Obviously this is widely out of scope for v12.

Greetings,

Andres Freund

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

Hi,

On 2019-08-02 18:38:46 -0400, Stephen Frost wrote:
> On Fri, Aug 2, 2019 at 18:27 Tom Lane  wrote:
> 
> > Tomas Vondra  writes:
> > > There seems to be a consensus that this this not a pg_basebackup issue
> > > (i.e. duplicate values don't make the file invalid), and it should be
> > > handled in ALTER SYSTEM.
> >
> > Yeah.  I doubt pg_basebackup is the only actor that can create such
> > situations.
> >
> > > The proposal seems to be to run through the .auto.conf file, remove any
> > > duplicates, and append the new entry at the end. That seems reasonable.
> >
> > +1

> I disagree that this should only be addressed in alter system, as I’ve said
> before and as others have agreed with.  Having one set of code that can be
> used to update parameters in the auto.conf and then have that be used by
> pg_basebackup, alter system, and external tools, is the right approach.
> 
> The idea that alter system should be the only thing that doesn’t just
> append changes to the file is just going to lead to confusion and bugs down
> the road.

To me that seems like an alternative that needs a good chunk more work
than just having ALTER SYSTEM fix things up, and isn't actually likely
to prevent such scenarios from occurring in practice.  Providing a
decent API to change conflict files from various places, presumably
including a commandline utility to do so, would be a nice feature, but
it seems vastly out of scope for v12.  My vote is to fix this via ALTER
SYSTEM in v12, and then for whoever is interested enough to provide
better tools down the road.

> As I said before, an alternative could be to make alter system simply
> always append and declare that to be the way to update parameters in the
> auto.conf.

Why would that be a good idea? We'd just take longer and longer to parse
it. There's people that change database settings on a regular and
automated basis using ALTER SYSTEm.

> > There was a discussion whether to print warnings about the duplicates. I
> > > personally see not much point in doing that - if we consider duplicates
> > > to be expected, and if ALTER SYSTEM has the license to rework the config
> > > file any way it wants, why warn about it?
> >
> > Personally I agree that warnings are unnecessary.

+1

Greetings,

Andres Freund

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

Stephen Frost  writes:
> On Fri, Aug 2, 2019 at 18:27 Tom Lane  wrote:
>>> The proposal seems to be to run through the .auto.conf file, remove any
>>> duplicates, and append the new entry at the end. That seems reasonable.

>> +1

> I disagree that this should only be addressed in alter system, as I’ve said
> before and as others have agreed with.  Having one set of code that can be
> used to update parameters in the auto.conf and then have that be used by
> pg_basebackup, alter system, and external tools, is the right approach.

I don't find that to be necessary or even desirable.  Many (most?) of the
situations where this would be important wouldn't have access to a running
backend, and maybe not to any PG code at all --- what if your tool isn't
written in C?

I think it's perfectly fine to say that external tools need only append
to the file, which will require no special tooling.  But then we need
ALTER SYSTEM to be willing to clean out duplicates, if only so you don't
run out of disk space after awhile.

regards, tom lane

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

On 8/3/19 7:27 AM, Tom Lane wrote:

Tomas Vondra writes:

There seems to be a consensus that this this not a pg_basebackup issue
(i.e. duplicate values don't make the file invalid), and it should be
handled in ALTER SYSTEM.

Yeah. I doubt pg_basebackup is the only actor that can create such
situations.

The proposal seems to be to run through the .auto.conf file, remove any
duplicates, and append the new entry at the end. That seems reasonable.

There was a discussion whether to print warnings about the duplicates. I
personally see not much point in doing that - if we consider duplicates
to be expected, and if ALTER SYSTEM has the license to rework the config
file any way it wants, why warn about it?

Personally I agree that warnings are unnecessary.

Having played around with the pg.auto.conf stuff for a while, my feeling is
that ALTER SYSTEM does indeed have a license to rewrite it (which is what
currently happens anyway, with comments and include directives [1] being
silently
removed) so it seems reasonable to remove duplicate entries and ensure
the correct one is processed.

[1] suprisingly any include directives present are honoured, which seems crazy
to me, see:
https://www.postgresql.org/message-id/flat/8c8bcbca-3bd9-dc6e-8986-04a5abdef142%402ndquadrant.com

The main issue however is that no code was written yet.

Seems like it ought to be relatively simple ... but I didn't look.

The patch I originally sent does exactly this.

The thread then drifted off into a discussion about providing ways for
applications to properly write to pg.auto.conf while PostgreSQL is not
running; I have a patch for that which I can submit later (though it
is a thing of considerable ugliness).

Regards

Ian Barwick

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

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

Greetings,

On Fri, Aug 2, 2019 at 18:27 Tom Lane  wrote:

> Tomas Vondra  writes:
> > There seems to be a consensus that this this not a pg_basebackup issue
> > (i.e. duplicate values don't make the file invalid), and it should be
> > handled in ALTER SYSTEM.
>
> Yeah.  I doubt pg_basebackup is the only actor that can create such
> situations.
>
> > The proposal seems to be to run through the .auto.conf file, remove any
> > duplicates, and append the new entry at the end. That seems reasonable.
>
> +1

I disagree that this should only be addressed in alter system, as I’ve said
before and as others have agreed with.  Having one set of code that can be
used to update parameters in the auto.conf and then have that be used by
pg_basebackup, alter system, and external tools, is the right approach.

The idea that alter system should be the only thing that doesn’t just
append changes to the file is just going to lead to confusion and bugs down
the road.

As I said before, an alternative could be to make alter system simply
always append and declare that to be the way to update parameters in the
auto.conf.

> There was a discussion whether to print warnings about the duplicates. I
> > personally see not much point in doing that - if we consider duplicates
> > to be expected, and if ALTER SYSTEM has the license to rework the config
> > file any way it wants, why warn about it?
>
> Personally I agree that warnings are unnecessary.

And at least Magnus and I disagree with that, as I recall from this
thread.  Let’s have a clean and clear way to modify the auto.conf and have
everything that touches the file update it in a consistent way.

Thanks,

Stephen

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

Tomas Vondra  writes:
> There seems to be a consensus that this this not a pg_basebackup issue
> (i.e. duplicate values don't make the file invalid), and it should be
> handled in ALTER SYSTEM.

Yeah.  I doubt pg_basebackup is the only actor that can create such
situations.

> The proposal seems to be to run through the .auto.conf file, remove any
> duplicates, and append the new entry at the end. That seems reasonable.

+1

> There was a discussion whether to print warnings about the duplicates. I
> personally see not much point in doing that - if we consider duplicates
> to be expected, and if ALTER SYSTEM has the license to rework the config
> file any way it wants, why warn about it?

Personally I agree that warnings are unnecessary.

> The main issue however is that no code was written yet.

Seems like it ought to be relatively simple ... but I didn't look.

regards, tom lane

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

2019-08-02 Thread Tomas Vondra


Hi,

This thread discusses an issue that's tracked as an open item for pg12,
but it's been quiet for the last ~1 month. I think it's probably time to
decide what to do with it. The thread is a bit long, so let me sum what
the issue is and what options we have.

The problem is that ALTER SYSTEM does not handle duplicate entries in
postgresql.auto.conf file correctly, because it simply modifies the
first item, but the value is then overridden by the duplicate items.
This contradicts the idea that duplicate GUCs are allowed, and that we
should use the last item.

This bug seems to exist since ALTER SYSTEM was introduced, so it's not
a clear PG12 item. But it was made more prominent by the removal of
recovery.conf in PG12, because pg_basebackup now appends stuff to
postgresql.auto.conf and may easily create duplicate items.


There seems to be a consensus that this this not a pg_basebackup issue
(i.e. duplicate values don't make the file invalid), and it should be
handled in ALTER SYSTEM.

The proposal seems to be to run through the .auto.conf file, remove any
duplicates, and append the new entry at the end. That seems reasonable.

There was a discussion whether to print warnings about the duplicates. I
personally see not much point in doing that - if we consider duplicates
to be expected, and if ALTER SYSTEM has the license to rework the config
file any way it wants, why warn about it?

The main issue however is that no code was written yet.


regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

2019-06-28 Thread Amit Kapila

On Tue, Jun 25, 2019 at 12:42 AM Stephen Frost  wrote:
>
> Greetings,
>
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
> > Robert Haas  writes:
> > > On Fri, Jun 21, 2019 at 12:55 PM Tom Lane  wrote:
> > >> Ah, got it.  So it seems like the correct behavior might be for
> > >> ALTER SYSTEM to
> > >> (a) run through the whole file and remove any conflicting lines;
> > >> (b) append new setting at the end.
> >
> > > That is exactly the behavior for which I am arguing.  Stephen also
> > > wants a warning, but I disagree, because the warning is totally
> > > non-actionable.  It tells you that some tool, at some point in the
> > > past, did something bad. You can't do anything about that, and you
> > > wouldn't need to except for the arbitrary decision to label duplicate
> > > lines as bad in the first place.
> >
> > Agreed; there's no particular reason to consider the situation as wrong.
> > guc.c has always had the policy that dups are fine and the last one wins.
> > The very design of ALTER SYSTEM owes its workability to that policy, so
> > we can hardly say that A.S. should have a different policy internally.
> >

Both are similar but not sure if they are the same because in A.S we
are planning to remove the duplicate entries from file whereas I think
in other places that rule is used to just ignore the duplicates and
allow the last one to win.   Now, I think there is merit in giving
WARNING in this case as we are intentionally removing something which
user has added it.  However,  it is not clear what user is going to do
with that WARNING unless we have a system where we detect such a
situation, give WARNING and then allow the user to proceed in this
case with some option like FORCE.

> > The problem here is simply that ALTER SYSTEM is failing to consider the
> > possibility that there are dups in postgresql.auto.conf, and that seems
> > like little more than an oversight to be fixed.
> >
> > There's more than one way we could implement a fix, perhaps, but I don't
> > really see a reason to work harder than is sketched above.
>
> Why bother removing the duplicate lines?
>
> If ALTER SYSTEM should remove them, why shouldn't other tools?
>
> > (BTW, has anyone checked whether ALTER SYSTEM RESET is prepared to remove
> > multiple lines for the same var?)
>
> No, it doesn't handle that today either, as discussed earlier in this
> thread.
>

Right, it doesn't handle that today, but I think we can deal it along
with Alter System Set ...

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

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

2019-06-28 Thread Amit Kapila

On Tue, Jun 25, 2019 at 7:31 AM Ian Barwick  wrote:
>
>  > In particular, in order to consider it unexpected, you have to suppose
>  >> that the content rules for postgresql.auto.conf are different from those
>  >> for postgresql.conf (wherein we clearly allow last-one-wins).  Can you
>  >> point to any user-facing documentation that says that?
>  >
>  > The backend and frontend tools don't modify postgresql.conf, and we
>  > don't document how to modify postgresql.auto.conf at *all*, even though
>  > we clearly now expect tool authors to go modifying it so that they can
>  > provide the same capabilities that pg_basebackup does and which they
>  > used to through recovery.conf, so I don't really see that as being
>  > comparable.
>  >
>  > The only thing we used to have to go on was what ALTER SYSTEM did, and
>  > then pg_basebackup went and did something different, and enough so that
>  > they ended up conflicting with each other, leading to this discussion.
>
> Or looking at it from another perspective - previously there was no
> particular use-case for appending to .auto.conf, until it (implicitly)
> became the only way of doing what recovery.conf used to do, and happened to
> expose the issue at hand.
>
> Leaving aside pg_basebackup and the whole issue of writing replication
> configuration, .auto.conf remains a text file which could potentially
> include duplicate entries, no matter how much we stipulate it shouldn't.
> As-is, ALTER SYSTEM fails to deal with this case, which in my opinion
> is a bug and a potential footgun which needs fixing.
>

I think there is an agreement that we should change it to remove
duplicates and add the new entry at the end.  However, we have not
reached an agreement on whether we should throw WARNING after removing
duplicates.

I think it is arguable that it was a bug in the first place in Alter
System as there is no way the duplicate lines can be there in
postgresql.auto.conf file before this feature or if someone ignores
the Warning on top of that file.   Having said that, I am in favor of
this change for the HEAD, but not sure if we should backpatch the same
as well by considering it as a bug-fix.

> (Though we'll still need to define/provide a way of writing configuration
> while the server is not running, which will be guaranteed to be read in last
> when it starts up).
>

Can you once verify if the current way of writing to
postgresql.auto.conf is safe in pg_basebackup?  It should ensure that
if there are any failures, partial wite problem while writing, then
the old file remains intact.  It is not clear to me if that is the
case with the current code of pg_basebackup, however the same is
ensured in Alter System code.  Because, if we haven't ensured it then
it is a problem for which we definitely need some fix.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

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

2019-06-25 Thread Sergei Kornilov

Hello

> But we already have ALTER SYSTEM, so why do we need to write it again?
> You just need to check whether the system is running: if it is, connect
> and do "ALTER SYSTEM". If it isn't, do `echo ALTER SYSTEM | postgres
> --single`.

Is this approach still possible for pg_basebackup --format=tar ? For 
"pg_basebackup -D - --format=tar" ?

regards, Sergei

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

2019-06-24 Thread Ian Barwick


> In particular, in order to consider it unexpected, you have to suppose
>> that the content rules for postgresql.auto.conf are different from those
>> for postgresql.conf (wherein we clearly allow last-one-wins).  Can you
>> point to any user-facing documentation that says that?
>
> The backend and frontend tools don't modify postgresql.conf, and we
> don't document how to modify postgresql.auto.conf at *all*, even though
> we clearly now expect tool authors to go modifying it so that they can
> provide the same capabilities that pg_basebackup does and which they
> used to through recovery.conf, so I don't really see that as being
> comparable.
>
> The only thing we used to have to go on was what ALTER SYSTEM did, and
> then pg_basebackup went and did something different, and enough so that
> they ended up conflicting with each other, leading to this discussion.

Or looking at it from another perspective - previously there was no
particular use-case for appending to .auto.conf, until it (implicitly)
became the only way of doing what recovery.conf used to do, and happened to
expose the issue at hand.

Leaving aside pg_basebackup and the whole issue of writing replication
configuration, .auto.conf remains a text file which could potentially
include duplicate entries, no matter how much we stipulate it shouldn't.
As-is, ALTER SYSTEM fails to deal with this case, which in my opinion
is a bug and a potential footgun which needs fixing.

(Though we'll still need to define/provide a way of writing configuration
while the server is not running, which will be guaranteed to be read in last
when it starts up).


Regards

Ian Barwick

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

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

2019-06-24 Thread Ian Barwick

On 6/25/19 4:06 AM, Alvaro Herrera wrote:
> On 2019-Jun-24, Robert Haas wrote:
>
>> On Sat, Jun 22, 2019 at 5:13 PM Stephen Frost  wrote:
>>> All that said, whatever code it is that we write for pg_basebackup to
>>> do this properly should go into our client side library, so other tools
>>> can leverage that and avoid having to write it themselves.
>>
>> That is probably only going to help people who are writing in C (or
>> maybe some close family member) and a lot of tools for managing
>> PostgreSQL will be written in scripting languages.
>
> But we already have ALTER SYSTEM, so why do we need to write it again?
> You just need to check whether the system is running: if it is, connect
> and do "ALTER SYSTEM".  If it isn't, do `echo ALTER SYSTEM | postgres
> --single`.  Maybe we can embed smarts to do that in, say, pg_ctl; then
> everybody has access to it.

Unfortunately, to quote the emitted log message, "standby mode is not
supported by single-user servers", which as-is renders this approach useless for
setting up replication configuration on a standby server (unless I'm missing
something).

I've looked in to what might be involved into creating a client-side function
for modifying .auto.conf while the system is not running, and basically
it seems to involve maintaining a stripped down version of ParseConfigFp()
which doesn't recurse (because we don't allow "include" directives in
.auto.conf, right? Right? I'll send in a different patch for that later...)
and somehow exposing write_auto_conf_file().

And for all those scripts which can't call the putative frontend C function,
we could provide a utility called "pg_alter_system" or similar which accepts
a name and a value and (provided the system is not running) "safely"
writes it to .auto.conf (though of course it won't be able to validate the
provided parameter(s)).

Alternatively (waves hand vaguely in air) there might be some way of
creating a single user startup mode for the express purpose of leveraging
the backend code to modify .auto.conf.

Bur that seems like a lot of effort and complexity to replace what, in Pg11
and earlier, was just a case of writing to recovery.conf.

Which brings me to another thought which AFAIK hasn't been discussed -
what use-cases are there for modifying .auto.conf when the system isn't
running?

The only one I can think of is the case at hand, i.e. configuring replication
after cloning a standby in a manner which *guarantees* that the
replication configuration will be read at startup, which was the case
with recovery.conf in Pg11 and earlier.

For anything else, it seems reasonable to me to expect any customised
settings to be written (e.g. by a provisioning system) to the normal
configuration file(s).

Having pg_basebackup write the replication configuration to a normal file
is icky because there's no guarantee the configuration will be written
last, or even included at all, which is a regression against earlier
versions as there you could clone a standby and (assuming there are no
issues with any cloned configuration files) have the standby start up
reliably.

Regards

Ian Barwick

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

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

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > Stephen and Magnus want a warning, because it's an indication that a
> > tool author, or *something* modified the file in an unexpected way, and
> > that we are having to do some kind of cleanup on the file because of it.
> 
> But you're presuming something that not everybody agrees with, which
> is that this situation should be considered unexpected.

And, at least at present, not everyone seems to be agreeing that having
duplicates should be considered expected, either.  Using only ALTER
SYSTEM, you'd never end up with duplicates either.

> In particular, in order to consider it unexpected, you have to suppose
> that the content rules for postgresql.auto.conf are different from those
> for postgresql.conf (wherein we clearly allow last-one-wins).  Can you
> point to any user-facing documentation that says that?

The backend and frontend tools don't modify postgresql.conf, and we
don't document how to modify postgresql.auto.conf at *all*, even though
we clearly now expect tool authors to go modifying it so that they can
provide the same capabilities that pg_basebackup does and which they
used to through recovery.conf, so I don't really see that as being
comparable.

The only thing we used to have to go on was what ALTER SYSTEM did, and
then pg_basebackup went and did something different, and enough so that
they ended up conflicting with each other, leading to this discussion.

Thanks,

Stephen

signature.asc
Description: PGP signature

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

Greetings,

* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> On 2019-Jun-24, Robert Haas wrote:
> 
> > On Sat, Jun 22, 2019 at 5:13 PM Stephen Frost  wrote:
> > > All that said, whatever code it is that we write for pg_basebackup to do 
> > > this properly should go into our client side library, so other tools can 
> > > leverage that and avoid having to write it themselves.
> > 
> > That is probably only going to help people who are writing in C (or
> > maybe some close family member) and a lot of tools for managing
> > PostgreSQL will be written in scripting languages.
> 
> But we already have ALTER SYSTEM, so why do we need to write it again?
> You just need to check whether the system is running: if it is, connect
> and do "ALTER SYSTEM".  If it isn't, do `echo ALTER SYSTEM | postgres
> --single`.  Maybe we can embed smarts to do that in, say, pg_ctl; then
> everybody has access to it.

While I'm not against adding some kind of support like that if we feel
like we really need it, I tend to think that just having it in
libpgcommon would be enough for most tool authors to use..

Thanks,

Stephen


signature.asc
Description: PGP signature

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

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Robert Haas  writes:
> > On Fri, Jun 21, 2019 at 12:55 PM Tom Lane  wrote:
> >> Ah, got it.  So it seems like the correct behavior might be for
> >> ALTER SYSTEM to
> >> (a) run through the whole file and remove any conflicting lines;
> >> (b) append new setting at the end.
> 
> > That is exactly the behavior for which I am arguing.  Stephen also
> > wants a warning, but I disagree, because the warning is totally
> > non-actionable.  It tells you that some tool, at some point in the
> > past, did something bad. You can't do anything about that, and you
> > wouldn't need to except for the arbitrary decision to label duplicate
> > lines as bad in the first place.
> 
> Agreed; there's no particular reason to consider the situation as wrong.
> guc.c has always had the policy that dups are fine and the last one wins.
> The very design of ALTER SYSTEM owes its workability to that policy, so
> we can hardly say that A.S. should have a different policy internally.
> 
> The problem here is simply that ALTER SYSTEM is failing to consider the
> possibility that there are dups in postgresql.auto.conf, and that seems
> like little more than an oversight to be fixed.
> 
> There's more than one way we could implement a fix, perhaps, but I don't
> really see a reason to work harder than is sketched above.

Why bother removing the duplicate lines?

If ALTER SYSTEM should remove them, why shouldn't other tools?

> (BTW, has anyone checked whether ALTER SYSTEM RESET is prepared to remove
> multiple lines for the same var?)

No, it doesn't handle that today either, as discussed earlier in this
thread.

If we want to get to should/must kind of language, then we could say
that tools should remove duplicated values, and must append to the end,
but I'm not sure that really changes things from what I'm proposing
anyway.

Thanks,

Stephen


signature.asc
Description: PGP signature

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

2019-06-24 Thread Tom Lane

Stephen Frost  writes:
> Stephen and Magnus want a warning, because it's an indication that a
> tool author, or *something* modified the file in an unexpected way, and
> that we are having to do some kind of cleanup on the file because of it.

But you're presuming something that not everybody agrees with, which
is that this situation should be considered unexpected.

In particular, in order to consider it unexpected, you have to suppose
that the content rules for postgresql.auto.conf are different from those
for postgresql.conf (wherein we clearly allow last-one-wins).  Can you
point to any user-facing documentation that says that?

regards, tom lane

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

2019-06-24 Thread Alvaro Herrera

On 2019-Jun-24, Robert Haas wrote:

> On Sat, Jun 22, 2019 at 5:13 PM Stephen Frost  wrote:
> > All that said, whatever code it is that we write for pg_basebackup to do 
> > this properly should go into our client side library, so other tools can 
> > leverage that and avoid having to write it themselves.
> 
> That is probably only going to help people who are writing in C (or
> maybe some close family member) and a lot of tools for managing
> PostgreSQL will be written in scripting languages.

But we already have ALTER SYSTEM, so why do we need to write it again?
You just need to check whether the system is running: if it is, connect
and do "ALTER SYSTEM".  If it isn't, do `echo ALTER SYSTEM | postgres
--single`.  Maybe we can embed smarts to do that in, say, pg_ctl; then
everybody has access to it.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

* Robert Haas (robertmh...@gmail.com) wrote:
> On Sat, Jun 22, 2019 at 5:13 PM Stephen Frost  wrote:
> > All that said, whatever code it is that we write for pg_basebackup to do 
> > this properly should go into our client side library, so other tools can 
> > leverage that and avoid having to write it themselves.
> 
> That is probably only going to help people who are writing in C (or
> maybe some close family member) and a lot of tools for managing
> PostgreSQL will be written in scripting languages.  It is unlikely
> that those people are going to get all of the rules for parsing a file
> full of GUC settings exactly right, because translating flex into
> Python is probably not anybody's idea of a fun time. So you'll end up
> with a bunch of rewrite-postgresql.auto.conf tools written in
> different languages at varying degrees of quality many of which will
> misfire in corner cases where the GUC names contain funny characters
> or the whitespace is off or there's unusual quoting involved.

Calling into C functions from Python certainly isn't new, nor is it
difficult to do from Perl, or various other languages, someone just
needs to write the bindings.  I'm not sure where the idea came from that
someone would translate flex into Python, that's certainly not what I
was suggesting at any point in this discussion.

> If you just decreed that it was OK to append to the file, you could
> avoid all that.

As I said elsewhere on this thread, I have absolutely no problem with
that as the documented approach to working with this file- but if that's
what we're going to have be the documented approach, then everything
should be using that approach...

Thanks,

Stephen

signature.asc
Description: PGP signature

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

Greetings,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Fri, Jun 21, 2019 at 12:55 PM Tom Lane  wrote:
> > Ah, got it.  So it seems like the correct behavior might be for
> > ALTER SYSTEM to
> > (a) run through the whole file and remove any conflicting lines;
> > (b) append new setting at the end.
> 
> That is exactly the behavior for which I am arguing.  Stephen also
> wants a warning, but I disagree, because the warning is totally
> non-actionable.  It tells you that some tool, at some point in the
> past, did something bad. You can't do anything about that, and you
> wouldn't need to except for the arbitrary decision to label duplicate
> lines as bad in the first place.

Stephen and Magnus want a warning, because it's an indication that a
tool author, or *something* modified the file in an unexpected way, and
that we are having to do some kind of cleanup on the file because of it.

If it was a tool author, who it certainly may very well be as they're
writing in support for the v12 changes, they'd almost certainly go and
fix their code to avoid doing that, lest users complain, which would be
exactly the behavior we want.

If it was the user themselves, which is also *entirely* likely, then
hopefully they'd realize that they really shouldn't be modifying that
file.

Thanks,

Stephen

signature.asc
Description: PGP signature

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

2019-06-24 Thread Tom Lane

Robert Haas  writes:
> On Fri, Jun 21, 2019 at 12:55 PM Tom Lane  wrote:
>> Ah, got it.  So it seems like the correct behavior might be for
>> ALTER SYSTEM to
>> (a) run through the whole file and remove any conflicting lines;
>> (b) append new setting at the end.

> That is exactly the behavior for which I am arguing.  Stephen also
> wants a warning, but I disagree, because the warning is totally
> non-actionable.  It tells you that some tool, at some point in the
> past, did something bad. You can't do anything about that, and you
> wouldn't need to except for the arbitrary decision to label duplicate
> lines as bad in the first place.

Agreed; there's no particular reason to consider the situation as wrong.
guc.c has always had the policy that dups are fine and the last one wins.
The very design of ALTER SYSTEM owes its workability to that policy, so
we can hardly say that A.S. should have a different policy internally.

The problem here is simply that ALTER SYSTEM is failing to consider the
possibility that there are dups in postgresql.auto.conf, and that seems
like little more than an oversight to be fixed.

There's more than one way we could implement a fix, perhaps, but I don't
really see a reason to work harder than is sketched above.

(BTW, has anyone checked whether ALTER SYSTEM RESET is prepared to remove
multiple lines for the same var?)

regards, tom lane

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