Обсуждение: [PATCH] Stop ALTER SYSTEM from making bad assumptions

Hi

Consider the following cascading standby setup with PostgreSQL 12:

- there exists a running primary "A"
- standby "B" is cloned from primary "A" using "pg_basebackup --write-recovery-conf"
- standby "C" is cloned from standby "B" using "pg_basebackup --write-recovery-conf"

So far, so good, everything works as expected.

Now, for whatever reason, the user wishes standby "C" to follow another upstream
node (which one is not important here), so the user, in the comfort of their own psql
command line (no more pesky recovery.conf editing!) issues the following:

     ALTER SYSTEM SET primary_conninfo = 'host=someothernode';

and restarts the instance, and... it stays connected to the original upstream node.

Which is unexpected.

Examining the the restarted instance, "SHOW primary_conninfo" still displays
the original value for "primary_conninfo". Mysteriouser and mysteriouser.

What has happened here is that with the option "--write-recovery-conf", Pg12's
pg_basebackup (correctly IMHO) appends the recovery settings to "postgresql.auto.conf".

However, on standby "C", pg_basebackup has dutifully copied over standby "B"'s
existing "postgresql.auto.conf", which already contains standby "B"'s
replication configuration, and appended standby "C"'s replication configuration
to that, which (before "ALTER SYSTEM" was invoked) looked something like this:

    # Do not edit this file manually!
    # It will be overwritten by the ALTER SYSTEM command.
    primary_conninfo = 'host=node_A'
    primary_conninfo = 'host=node_B'

which is expected, and works because the last entry in the file is evaluated, so
on startup, standby "C" follows standby "B".

However, executing "ALTER SYSTEM SET primary_conninfo = 'host=someothernode'" left
standby "C"'s "postgresql.auto.conf" file looking like this:

    # Do not edit this file manually!
    # It will be overwritten by the ALTER SYSTEM command.
    primary_conninfo = 'host=someothernode'
    primary_conninfo = 'host=node_B'

which seems somewhat broken, to say the least.

As-is, the user will either need to repeatedly issue "ALTER SYSTEM RESET primary_conninfo"
until the duplicates are cleared (which means "ALTER SYSTEM RESET ..." doesn't work as
advertised, and is not an obvious solution anyway), or ignore the "Do not edit this file manually!"
warning and remove the offending entry/entries (which, if done safely, should involve
shutting down the instance first).

Note this issue is not specific to pg_basebackup, primary_conninfo (or any other settings
formerly in recovery.conf), it has just manifested itself as the built-in toolset as of now
provides a handy way of getting into this situation without too much effort (and any
utilities which clone standbys and append the replication configuration to
"postgresql.auto.conf" in lieu of creating "recovery.conf" will be prone to running into
the same situation).

I had previously always assumed that ALTER SYSTEM  would change the *last* occurrence for
the parameter in "postgresql.auto.conf", in the same way you'd need to be sure to change
the last occurrence in the normal configuration files, however this actually not the case -
as per replace_auto_config_value() ( src/backend/utils/misc/guc.c ):

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

the *first* match is replaced.

Attached patch attempts to rectify this situation by having replace_auto_config_value()
deleting any duplicate entries first, before making any changes to the last entry.

Arguably it might be sufficient (and simpler) to just scan the list for the last
entry, but removing preceding duplicates seems cleaner, as it's pointless
(and a potential source of confusion) keeping entries around which will never be used.

Also attached is a set of TAP tests to check ALTER SYSTEM works as expected (or
at least as seems correct to me).


Regards

Ian Barwick

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

Greetings,

* Ian Barwick (ian.barwick@2ndquadrant.com) wrote:
> Consider the following cascading standby setup with PostgreSQL 12:
>
> - there exists a running primary "A"
> - standby "B" is cloned from primary "A" using "pg_basebackup --write-recovery-conf"
> - standby "C" is cloned from standby "B" using "pg_basebackup --write-recovery-conf"
>
> So far, so good, everything works as expected.
>
> Now, for whatever reason, the user wishes standby "C" to follow another upstream
> node (which one is not important here), so the user, in the comfort of their own psql
> command line (no more pesky recovery.conf editing!) issues the following:
>
>     ALTER SYSTEM SET primary_conninfo = 'host=someothernode';
>
> and restarts the instance, and... it stays connected to the original upstream node.
>
> Which is unexpected.
>
> Examining the the restarted instance, "SHOW primary_conninfo" still displays
> the original value for "primary_conninfo". Mysteriouser and mysteriouser.
>
> What has happened here is that with the option "--write-recovery-conf", Pg12's
> pg_basebackup (correctly IMHO) appends the recovery settings to "postgresql.auto.conf".
>
> However, on standby "C", pg_basebackup has dutifully copied over standby "B"'s
> existing "postgresql.auto.conf", which already contains standby "B"'s
> replication configuration, and appended standby "C"'s replication configuration
> to that, which (before "ALTER SYSTEM" was invoked) looked something like this:
>
>     # Do not edit this file manually!
>     # It will be overwritten by the ALTER SYSTEM command.
>     primary_conninfo = 'host=node_A'
>     primary_conninfo = 'host=node_B'
>
> which is expected, and works because the last entry in the file is evaluated, so
> on startup, standby "C" follows standby "B".
>
> However, executing "ALTER SYSTEM SET primary_conninfo = 'host=someothernode'" left
> standby "C"'s "postgresql.auto.conf" file looking like this:
>
>     # Do not edit this file manually!
>     # It will be overwritten by the ALTER SYSTEM command.
>     primary_conninfo = 'host=someothernode'
>     primary_conninfo = 'host=node_B'
>
> which seems somewhat broken, to say the least.

Yes, it's completely broken, which I've complained about at least twice
on this list to no avail.

Thanks for putting together an example case pointing out why it's a
serious issue.  The right thing to do here it so create an open item for
PG12 around this.

> As-is, the user will either need to repeatedly issue "ALTER SYSTEM RESET primary_conninfo"
> until the duplicates are cleared (which means "ALTER SYSTEM RESET ..." doesn't work as
> advertised, and is not an obvious solution anyway), or ignore the "Do not edit this file manually!"
> warning and remove the offending entry/entries (which, if done safely, should involve
> shutting down the instance first).
>
> Note this issue is not specific to pg_basebackup, primary_conninfo (or any other settings
> formerly in recovery.conf), it has just manifested itself as the built-in toolset as of now
> provides a handy way of getting into this situation without too much effort (and any
> utilities which clone standbys and append the replication configuration to
> "postgresql.auto.conf" in lieu of creating "recovery.conf" will be prone to running into
> the same situation).

This is absolutely the fault of the system for putting in multiple
entries into the auto.conf, which it wasn't ever written to handle.

> I had previously always assumed that ALTER SYSTEM  would change the *last* occurrence for
> the parameter in "postgresql.auto.conf", in the same way you'd need to be sure to change
> the last occurrence in the normal configuration files, however this actually not the case -
> as per replace_auto_config_value() ( src/backend/utils/misc/guc.c ):
>
>     /* Search the list for an existing match (we assume there's only one) */
>
> the *first* match is replaced.
>
> Attached patch attempts to rectify this situation by having replace_auto_config_value()
> deleting any duplicate entries first, before making any changes to the last entry.

While this might be a good belt-and-suspenders kind of change to
include, I don't think pg_basebackup should be causing us to have
multiple entries in the file in the first place..

> Arguably it might be sufficient (and simpler) to just scan the list for the last
> entry, but removing preceding duplicates seems cleaner, as it's pointless
> (and a potential source of confusion) keeping entries around which will never be used.

I don't think we should only change the last entry, that seems like a
really bad idea.  I agree that we should clean up the file if we come
across it being invalid.

> Also attached is a set of TAP tests to check ALTER SYSTEM works as expected (or
> at least as seems correct to me).

In my view, at least, we should have a similar test for pg_basebackup to
make sure that it doesn't create an invalid .auto.conf file.

Thanks!

Stephen

On Fri, Jun 14, 2019 at 9:38 PM Stephen Frost <sfrost@snowman.net> wrote:
> * Ian Barwick (ian.barwick@2ndquadrant.com) wrote:
> >
> > Note this issue is not specific to pg_basebackup, primary_conninfo (or any other settings
> > formerly in recovery.conf), it has just manifested itself as the built-in toolset as of now
> > provides a handy way of getting into this situation without too much effort (and any
> > utilities which clone standbys and append the replication configuration to
> > "postgresql.auto.conf" in lieu of creating "recovery.conf" will be prone to running into
> > the same situation).
>
> This is absolutely the fault of the system for putting in multiple
> entries into the auto.conf, which it wasn't ever written to handle.
>

Right.  I think if possible, it should use existing infrastructure to
write to postgresql.auto.conf rather than inventing a new way to
change it.  Apart from this issue, if we support multiple ways to edit
postgresql.auto.conf, we might end up with more problems like this in
the future where one system is not aware of the way file being edited
by another system.

> > I had previously always assumed that ALTER SYSTEM  would change the *last* occurrence for
> > the parameter in "postgresql.auto.conf", in the same way you'd need to be sure to change
> > the last occurrence in the normal configuration files, however this actually not the case -
> > as per replace_auto_config_value() ( src/backend/utils/misc/guc.c ):
> >
> >     /* Search the list for an existing match (we assume there's only one) */
> >
> > the *first* match is replaced.
> >
> > Attached patch attempts to rectify this situation by having replace_auto_config_value()
> > deleting any duplicate entries first, before making any changes to the last entry.
>
> While this might be a good belt-and-suspenders kind of change to
> include,
>

Another possibility to do something on these lines is to extend Alter
System Reset <config_param> to remove all the duplicate entries.  Then
the user has a way to remove all duplicate entries if any and set the
new value.  I think handling duplicate entries in *.auto.conf files is
an enhancement of the existing system and there could be multiple
things we can do there, so we shouldn't try to do that as a bug-fix.

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

Greetings,

* Amit Kapila (amit.kapila16@gmail.com) wrote:
> On Fri, Jun 14, 2019 at 9:38 PM Stephen Frost <sfrost@snowman.net> wrote:
> > * Ian Barwick (ian.barwick@2ndquadrant.com) wrote:
> > >
> > > Note this issue is not specific to pg_basebackup, primary_conninfo (or any other settings
> > > formerly in recovery.conf), it has just manifested itself as the built-in toolset as of now
> > > provides a handy way of getting into this situation without too much effort (and any
> > > utilities which clone standbys and append the replication configuration to
> > > "postgresql.auto.conf" in lieu of creating "recovery.conf" will be prone to running into
> > > the same situation).
> >
> > This is absolutely the fault of the system for putting in multiple
> > entries into the auto.conf, which it wasn't ever written to handle.
>
> Right.  I think if possible, it should use existing infrastructure to
> write to postgresql.auto.conf rather than inventing a new way to
> change it.  Apart from this issue, if we support multiple ways to edit
> postgresql.auto.conf, we might end up with more problems like this in
> the future where one system is not aware of the way file being edited
> by another system.

I agere that there should have been some effort put into making the way
ALTER SYSTEM is modified be consistent between the backend and utilities
like pg_basebackup (which would also help third party tools understand
how a non-backend application should be modifying the file).

> > > I had previously always assumed that ALTER SYSTEM  would change the *last* occurrence for
> > > the parameter in "postgresql.auto.conf", in the same way you'd need to be sure to change
> > > the last occurrence in the normal configuration files, however this actually not the case -
> > > as per replace_auto_config_value() ( src/backend/utils/misc/guc.c ):
> > >
> > >     /* Search the list for an existing match (we assume there's only one) */
> > >
> > > the *first* match is replaced.
> > >
> > > Attached patch attempts to rectify this situation by having replace_auto_config_value()
> > > deleting any duplicate entries first, before making any changes to the last entry.
> >
> > While this might be a good belt-and-suspenders kind of change to
> > include,
>
> Another possibility to do something on these lines is to extend Alter
> System Reset <config_param> to remove all the duplicate entries.  Then
> the user has a way to remove all duplicate entries if any and set the
> new value.  I think handling duplicate entries in *.auto.conf files is
> an enhancement of the existing system and there could be multiple
> things we can do there, so we shouldn't try to do that as a bug-fix.

Unless there's actually a use-case for duplicate entries in
postgresql.auto.conf, what we should do is clean them up (and possibly
throw a WARNING or similar at the user saying "something modified your
postgresql.auto.conf in an unexpected way").  I'd suggest we do that on
every ALTER SYSTEM call.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> Unless there's actually a use-case for duplicate entries in
> postgresql.auto.conf,

There isn't --- guc.c will just discard the earlier duplicates.

> what we should do is clean them up (and possibly
> throw a WARNING or similar at the user saying "something modified your
> postgresql.auto.conf in an unexpected way").  I'd suggest we do that on
> every ALTER SYSTEM call.

+1 for having ALTER SYSTEM clean out duplicates.  Not sure whether
a WARNING would seem too in-your-face.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > Unless there's actually a use-case for duplicate entries in
> > postgresql.auto.conf,
>
> There isn't --- guc.c will just discard the earlier duplicates.

One might be able to argue for trying to create a stack or some such, to
allow you to more easily move between values or to see what the value
was set to at some point in the past, etc etc.  Until we see an actual
thought out use-case along those lines that requires supporting
duplicates in some fashion though, with code to make it all work, I
don't think we should allow it.

> > what we should do is clean them up (and possibly
> > throw a WARNING or similar at the user saying "something modified your
> > postgresql.auto.conf in an unexpected way").  I'd suggest we do that on
> > every ALTER SYSTEM call.
>
> +1 for having ALTER SYSTEM clean out duplicates.  Not sure whether
> a WARNING would seem too in-your-face.

I'd hope for a warning from basically every part of the system when it
detects, clearly, that a file was changed in a way that it shouldn't
have been.  If we don't throw a warning, then we're implying that it's
acceptable, but then cleaning up the duplicates, which seems pretty
confusing.

Thanks,

Stephen

On 17/06/2019 05:58, Magnus Hagander wrote:
> On Sun, Jun 16, 2019 at 7:43 PM Stephen Frost <sfrost@snowman.net 
> <mailto:sfrost@snowman.net>> wrote:
>
>
>     * Tom Lane (tgl@sss.pgh.pa.us <mailto:tgl@sss.pgh.pa.us>) wrote:
>     > Stephen Frost <sfrost@snowman.net <mailto:sfrost@snowman.net>>
>     writes:
>
>     > > what we should do is clean them up (and possibly
>     > > throw a WARNING or similar at the user saying "something
>     modified your
>     > > postgresql.auto.conf in an unexpected way").  I'd suggest we
>     do that on
>     > > every ALTER SYSTEM call.
>     >
>     > +1 for having ALTER SYSTEM clean out duplicates.  Not sure whether
>     > a WARNING would seem too in-your-face.
>
>     I'd hope for a warning from basically every part of the system when it
>     detects, clearly, that a file was changed in a way that it shouldn't
>     have been.  If we don't throw a warning, then we're implying that it's
>     acceptable, but then cleaning up the duplicates, which seems pretty
>     confusing.
>
>
> +1. Silently "fixing" the file by cleaning up duplicates is going to 
> be even more confusing to uses who had seen them be there before.
>
> -- 
>  Magnus Hagander
>  Me: https://www.hagander.net/ <http://www.hagander.net/>
>  Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

I thinking fixing this silently should be at least a hanging offence.

At one time I came a cross a language PL/1, that would silently 
'correct' some mistakes, without indicating what it did.  I thought this 
was extremely dangerous, that could lead to some very nasty and 
unexpected bugs!

It is most important that people be aware of possibly conflicting 
changes, or that values they saw in postgresql.conf may have been changed.

Hmm... this suggests that all the implied defaults should be explicitly 
set!  Would that be too greater change to make?


Cheers,
Gavin

On Mon, Jun 17, 2019 at 12:57 AM Gavin Flower
<GavinFlower@archidevsys.co.nz> wrote:
>
>
> I thinking fixing this silently should be at least a hanging offence.
>

Maybe adding a MD5 header to the file to check if it has been altered
outside guc.c might be enough.

Regards,

Juan José Santamaría Flecha

Hi

On 6/15/19 1:08 AM, Stephen Frost wrote:
 > Greetings,
 >
 > * Ian Barwick (ian.barwick@2ndquadrant.com) wrote:
 >> Consider the following cascading standby setup with PostgreSQL 12:
 >>
 >> - there exists a running primary "A"
 >> - standby "B" is cloned from primary "A" using "pg_basebackup --write-recovery-conf"
 >> - standby "C" is cloned from standby "B" using "pg_basebackup --write-recovery-conf"
(...)
 >> However, executing "ALTER SYSTEM SET primary_conninfo = 'host=someothernode'" left
 >> standby "C"'s "postgresql.auto.conf" file looking like this:
 >>
 >>     # Do not edit this file manually!
 >>     # It will be overwritten by the ALTER SYSTEM command.
 >>     primary_conninfo = 'host=someothernode'
 >>     primary_conninfo = 'host=node_B'
 >>
 >> which seems somewhat broken, to say the least.
 >
 > Yes, it's completely broken, which I've complained about at least twice
 > on this list to no avail.
 >
 > Thanks for putting together an example case pointing out why it's a
 > serious issue.  The right thing to do here it so create an open item for
 > PG12 around this.

Done.

 >> Attached patch attempts to rectify this situation by having replace_auto_config_value()
 >> deleting any duplicate entries first, before making any changes to the last entry.
 >
 > While this might be a good belt-and-suspenders kind of change to
 > include, I don't think pg_basebackup should be causing us to have
 > multiple entries in the file in the first place..
(...)
 >> Also attached is a set of TAP tests to check ALTER SYSTEM works as expected (or
 >> at least as seems correct to me).
 >
 > In my view, at least, we should have a similar test for pg_basebackup to
 > make sure that it doesn't create an invalid .auto.conf file.

Indeed... I'd be happy to create tests... but first we need a definition of what
constitutes a valid .auto.conf file.

If that definition includes requiring that a parameter may occur only once, then
we need to provide a way for utilities such as pg_basebackup to write the replication
configuration to a configuration file in such a way that it doesn't somehow
render that file invalid.

In Pg11 and earlier, it was just a case of writing (or overwriting) recovery.conf.

In Pg12, the code in pg_basebackup implies the correct thing to do is append to .auto.conf,
but as demonstrated that can cause problems with duplicate entries.

Having pg_basebackup, or any other utility which clones a standby, parse the file
itself to remove duplicates seems like a recipe for pain and badly duplicated effort
(unless there's some way of calling the configuration parsing code while the
server is not running).

We could declare that the .auto.conf file will be reset to the default state when
a standby is cloned, but the implicit behaviour so far has been to copy the file
as-is (as would happen with any other configuration files in the data directory).

We could avoid the need for modifying the .auto.conf file by declaring that a
configuration with a specific name in the data directory (let's call it
"recovery.conf" or "replication.conf") can be used by any utilities writing
replication configuration (though of course in contrast to the old recovery.conf
it would be included, if exists, as a normal configuration file, though then the
precedence would need to be defined, etc..). I'm not sure off the top of my head
whether something like that has already been discussed, though it's presumably a
bit late in the release cycle to make such changes anyway?

 >>> This is absolutely the fault of the system for putting in multiple
 >>> entries into the auto.conf, which it wasn't ever written to handle.
 >>
 > * Amit Kapila (amit.kapila16@gmail.com) wrote:
 >> Right.  I think if possible, it should use existing infrastructure to
 >> write to postgresql.auto.conf rather than inventing a new way to
 >> change it.  Apart from this issue, if we support multiple ways to edit
 >> postgresql.auto.conf, we might end up with more problems like this in
 >> the future where one system is not aware of the way file being edited
 >> by another system.
 >
 > I agere that there should have been some effort put into making the way
 > ALTER SYSTEM is modified be consistent between the backend and utilities
 > like pg_basebackup (which would also help third party tools understand
 > how a non-backend application should be modifying the file).

Did you mean to say "the way postgresql.auto.conf is modified"?

I suggest explicitly documenting postgresql.auto.conf behaviour (and the circumstances
where it's acceptable to modify it outside of ALTER SYSTEM calls) in the documentation
(and possibly in the code), so anyone writing utilities which need to
append to postgresql.auto.conf knows what the situation is.

Something along the following lines?

- postgresql.auto.conf is maintained by PostgreSQL and can be rewritten at will by the system
   at any time
- there is no guarantee that items in postgresql.auto.conf will be in a particular order
- it  must never be manually edited (though it may be viewed)
- postgresql.auto.conf may be appended to by utilities which need to write configuration
   items and which and need a guarantee that the items will be read by the server at startup
   (but only when the server is down of course)
- any duplicate items will be removed when ALTER SYSTEM is executed to change or reset
   an item (a WARNING will be emitted about duplicate items removed)
- comment lines (apart from the warning at the top of the file) will be silently removed
   (this is currently the case anyway)


I will happily work on those changes in the next few days if agreed.



Regards

Ian Barwick

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

On 6/17/19 2:58 AM, Magnus Hagander wrote:
> On Sun, Jun 16, 2019 at 7:43 PM Stephen Frost <sfrost@snowman.net <mailto:sfrost@snowman.net>> wrote:
> 
> 
>     * Tom Lane (tgl@sss.pgh.pa.us <mailto:tgl@sss.pgh.pa.us>) wrote:
>      > Stephen Frost <sfrost@snowman.net <mailto:sfrost@snowman.net>> writes:
> 
>      > > what we should do is clean them up (and possibly
>      > > throw a WARNING or similar at the user saying "something modified your
>      > > postgresql.auto.conf in an unexpected way").  I'd suggest we do that on
>      > > every ALTER SYSTEM call.
>      >
>      > +1 for having ALTER SYSTEM clean out duplicates.  Not sure whether
>      > a WARNING would seem too in-your-face.
> 
>     I'd hope for a warning from basically every part of the system when it
>     detects, clearly, that a file was changed in a way that it shouldn't
>     have been.  If we don't throw a warning, then we're implying that it's
>     acceptable, but then cleaning up the duplicates, which seems pretty
>     confusing.
> 
> > +1. Silently "fixing" the file by cleaning up duplicates is going to be even
 >  more confusing o uses who had seen them be there before.

Some sort of notification is definitely appropriate here.

However, going back to the original scenario (cascaded standby set up using
"pg_basebackup --write-recovery-conf") there would now be a warning emitted
the first time anyone executes ALTER SYSTEM (about duplicate "primary_conninfo"
entries) which would not have occured in Pg11 and earlier (and which will
no doubt cause consternation along the lines "how did my postgresql.auto.conf
get modified in an unexpected way? OMG? Bug? Was I hacked?").


Regards

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

Greetings,

* Ian Barwick (ian.barwick@2ndquadrant.com) wrote:
> However, going back to the original scenario (cascaded standby set up using
> "pg_basebackup --write-recovery-conf") there would now be a warning emitted
> the first time anyone executes ALTER SYSTEM (about duplicate "primary_conninfo"
> entries) which would not have occured in Pg11 and earlier (and which will
> no doubt cause consternation along the lines "how did my postgresql.auto.conf
> get modified in an unexpected way? OMG? Bug? Was I hacked?").

No, I don't think we should end up in a situation where this happens.

I agree that this implies making pg_basebackup more intelligent when
it's dealing with that file but I simply don't have a lot of sympathy
about that, it's not news to anyone who has been paying attention.

Thanks,

Stephen

Greetings,

* Ian Barwick (ian.barwick@2ndquadrant.com) wrote:
> On 6/15/19 1:08 AM, Stephen Frost wrote:
> > * Ian Barwick (ian.barwick@2ndquadrant.com) wrote:
> >> Consider the following cascading standby setup with PostgreSQL 12:
> >>
> >> - there exists a running primary "A"
> >> - standby "B" is cloned from primary "A" using "pg_basebackup --write-recovery-conf"
> >> - standby "C" is cloned from standby "B" using "pg_basebackup --write-recovery-conf"
> (...)
> >> However, executing "ALTER SYSTEM SET primary_conninfo = 'host=someothernode'" left
> >> standby "C"'s "postgresql.auto.conf" file looking like this:
> >>
> >>     # Do not edit this file manually!
> >>     # It will be overwritten by the ALTER SYSTEM command.
> >>     primary_conninfo = 'host=someothernode'
> >>     primary_conninfo = 'host=node_B'
> >>
> >> which seems somewhat broken, to say the least.
> >
> > Yes, it's completely broken, which I've complained about at least twice
> > on this list to no avail.
> >
> > Thanks for putting together an example case pointing out why it's a
> > serious issue.  The right thing to do here it so create an open item for
> > PG12 around this.
>
> Done.

Thanks.

> >> Attached patch attempts to rectify this situation by having replace_auto_config_value()
> >> deleting any duplicate entries first, before making any changes to the last entry.
> >
> > While this might be a good belt-and-suspenders kind of change to
> > include, I don't think pg_basebackup should be causing us to have
> > multiple entries in the file in the first place..
> (...)
> >> Also attached is a set of TAP tests to check ALTER SYSTEM works as expected (or
> >> at least as seems correct to me).
> >
> > In my view, at least, we should have a similar test for pg_basebackup to
> > make sure that it doesn't create an invalid .auto.conf file.
>
> Indeed... I'd be happy to create tests... but first we need a definition of what
> constitutes a valid .auto.conf file.
>
> If that definition includes requiring that a parameter may occur only once, then
> we need to provide a way for utilities such as pg_basebackup to write the replication
> configuration to a configuration file in such a way that it doesn't somehow
> render that file invalid.

Yes, I think that we do need to require that a parameter only occur once
and pg_basebackup and friends need to be able to manage that.

> In Pg11 and earlier, it was just a case of writing (or overwriting) recovery.conf.

Right.

> In Pg12, the code in pg_basebackup implies the correct thing to do is append to .auto.conf,
> but as demonstrated that can cause problems with duplicate entries.

Code can have bugs. :)  I'd argue that this is such a bug that needs to
be fixed..

> Having pg_basebackup, or any other utility which clones a standby, parse the file
> itself to remove duplicates seems like a recipe for pain and badly duplicated effort
> (unless there's some way of calling the configuration parsing code while the
> server is not running).

I don't really see that there's much hope for it.

> We could declare that the .auto.conf file will be reset to the default state when
> a standby is cloned, but the implicit behaviour so far has been to copy the file
> as-is (as would happen with any other configuration files in the data directory).
>
> We could avoid the need for modifying the .auto.conf file by declaring that a
> configuration with a specific name in the data directory (let's call it
> "recovery.conf" or "replication.conf") can be used by any utilities writing
> replication configuration (though of course in contrast to the old recovery.conf
> it would be included, if exists, as a normal configuration file, though then the
> precedence would need to be defined, etc..). I'm not sure off the top of my head
> whether something like that has already been discussed, though it's presumably a
> bit late in the release cycle to make such changes anyway?

This was discussed a fair bit, including suggestions along exactly those
lines.  There were various arguments for and against, so you might want
to review the threads where that discussion happened to see what the
reasoning was for not having such an independent file.

> >>> This is absolutely the fault of the system for putting in multiple
> >>> entries into the auto.conf, which it wasn't ever written to handle.
> >>
> > * Amit Kapila (amit.kapila16@gmail.com) wrote:
> >> Right.  I think if possible, it should use existing infrastructure to
> >> write to postgresql.auto.conf rather than inventing a new way to
> >> change it.  Apart from this issue, if we support multiple ways to edit
> >> postgresql.auto.conf, we might end up with more problems like this in
> >> the future where one system is not aware of the way file being edited
> >> by another system.
> >
> > I agere that there should have been some effort put into making the way
> > ALTER SYSTEM is modified be consistent between the backend and utilities
> > like pg_basebackup (which would also help third party tools understand
> > how a non-backend application should be modifying the file).
>
> Did you mean to say "the way postgresql.auto.conf is modified"?

Ah, yes, more-or-less.  I think I was going for 'the way ALTER SYSTEM
modifies postgresql.auto.conf'.

> I suggest explicitly documenting postgresql.auto.conf behaviour (and the circumstances
> where it's acceptable to modify it outside of ALTER SYSTEM calls) in the documentation
> (and possibly in the code), so anyone writing utilities which need to
> append to postgresql.auto.conf knows what the situation is.

Yeah, I would think that, ideally, we'd have some code in the common
library that other utilities could leverage and which the backend would
also use.

> - postgresql.auto.conf is maintained by PostgreSQL and can be rewritten at will by the system
>   at any time

I'd further say something along the lines of 'utilities should not
modify a postgresql.auto.conf that's in place under a running PostgreSQL
cluster'.

> - there is no guarantee that items in postgresql.auto.conf will be in a particular order
> - it  must never be manually edited (though it may be viewed)

'must' is perhaps a bit strong...  I would say something like
"shouldn't", as users may *have* to modify it, if PostgreSQL won't
start due to some configuration in it.

> - postgresql.auto.conf may be appended to by utilities which need to write configuration
>   items and which and need a guarantee that the items will be read by the server at startup
>   (but only when the server is down of course)

Well, I wouldn't support saying "append" since that's what got us into
this mess. :)

> - any duplicate items will be removed when ALTER SYSTEM is executed to change or reset
>   an item (a WARNING will be emitted about duplicate items removed)
> - comment lines (apart from the warning at the top of the file) will be silently removed
>   (this is currently the case anyway)

I'd rather say that 'any duplicate items should be removed, and a
WARNING emitted when detected', or something along those lines.  Same
for comment lines...

> I will happily work on those changes in the next few days if agreed.

Great!

Thanks,

Stephen

Greetings,

* Magnus Hagander (magnus@hagander.net) wrote:
> On Mon, Jun 17, 2019 at 5:41 PM Stephen Frost <sfrost@snowman.net> wrote:
> > I'd further say something along the lines of 'utilities should not
> > modify a postgresql.auto.conf that's in place under a running PostgreSQL
> > cluster'.
>
> Do we need to differ between "external" and "internal" utilities here?

I don't think so..?  Is there something there that you're thinking would
be different between them?

> > I'd rather say that 'any duplicate items should be removed, and a
> > WARNING emitted when detected', or something along those lines.  Same
> > for comment lines...
>
> I think it's perfectly fine to silently drop comments (other than the one
> at the very top which says don't touch this file).

I'm not sure why that's different?  I don't really think that I agree
with you on this one- anything showing up in that file that we're ending
up removing must have gotten there because someone or something didn't
realize the rules around managing the file, and that's a problem...

Thanks,

Stephen

On Mon, Jun 17, 2019 at 8:20 PM Ian Barwick <ian.barwick@2ndquadrant.com> wrote:
> On 6/15/19 1:08 AM, Stephen Frost wrote:
>  > * Amit Kapila (amit.kapila16@gmail.com) wrote:
>  >> Right.  I think if possible, it should use existing infrastructure to
>  >> write to postgresql.auto.conf rather than inventing a new way to
>  >> change it.  Apart from this issue, if we support multiple ways to edit
>  >> postgresql.auto.conf, we might end up with more problems like this in
>  >> the future where one system is not aware of the way file being edited
>  >> by another system.
>  >
>  > I agere that there should have been some effort put into making the way
>  > ALTER SYSTEM is modified be consistent between the backend and utilities
>  > like pg_basebackup (which would also help third party tools understand
>  > how a non-backend application should be modifying the file).
>
> Did you mean to say "the way postgresql.auto.conf is modified"?
>

Yes, that is what we are discussing here.  I think what we can do here
is to extract the functionality to set the parameter in .auto.conf
from AlterSystemSetConfigFile and expose it via a function that takes
(option_name, value) as a parameter.  Then we can expose it via some
SQL function like set_auto_config (similar to what we have now for
set_config/set_config_by_name).  I think if we have something like
that then pg_basebackup or any other utility can use it in a
consistent way.

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

On 6/19/19 12:46 PM, Amit Kapila wrote:
> On Mon, Jun 17, 2019 at 8:20 PM Ian Barwick <ian.barwick@2ndquadrant.com> wrote:
>> On 6/15/19 1:08 AM, Stephen Frost wrote:
>>   > * Amit Kapila (amit.kapila16@gmail.com) wrote:
>>   >> Right.  I think if possible, it should use existing infrastructure to
>>   >> write to postgresql.auto.conf rather than inventing a new way to
>>   >> change it.  Apart from this issue, if we support multiple ways to edit
>>   >> postgresql.auto.conf, we might end up with more problems like this in
>>   >> the future where one system is not aware of the way file being edited
>>   >> by another system.
>>   >
>>   > I agere that there should have been some effort put into making the way
>>   > ALTER SYSTEM is modified be consistent between the backend and utilities
>>   > like pg_basebackup (which would also help third party tools understand
>>   > how a non-backend application should be modifying the file).
>>
>> Did you mean to say "the way postgresql.auto.conf is modified"?
>>
> 
> Yes, that is what we are discussing here.  I think what we can do here
> is to extract the functionality to set the parameter in .auto.conf
> from AlterSystemSetConfigFile and expose it via a function that takes
> (option_name, value) as a parameter.

Yup, I was just considering what's involved there, will reply to another
mail in the thread on that.

> Then we can expose it via some
> SQL function like set_auto_config (similar to what we have now for
> set_config/set_config_by_name).  I think if we have something like
> that then pg_basebackup or any other utility can use it in a
> consistent way.

Umm, but the point is here, the server will *not* be running at this point,
so calling an SQL function is out of the question. And if the server
is running, then you just execute "ALTER SYSTEM".


Regards

Ian Barwick

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

n 6/18/19 12:41 AM, Stephen Frost wrote:
 > Greetings,
 >
 > * Ian Barwick (ian.barwick@2ndquadrant.com) wrote
(...)

 >> I suggest explicitly documenting postgresql.auto.conf behaviour (and the circumstances
 >> where it's acceptable to modify it outside of ALTER SYSTEM calls) in the documentation
 >> (and possibly in the code), so anyone writing utilities which need to
 >> append to postgresql.auto.conf knows what the situation is.
 >
 > Yeah, I would think that, ideally, we'd have some code in the common
 > library that other utilities could leverage and which the backend would
 > also use.

So maybe something along the lines of creating a stripped-down variant of
AlterSystemSetConfigFile() (from "src/backend/utils/misc/guc.c") which can be
called from front-end code to safely modify .auto.conf while the server is *not*
running.

I'm not terribly familiar with the GUC code, but that would presumably mean making
parts of the GUC parsing/handling code linkable externally (ParseConfigFp() etc.)
as you'd need to parse the file before rewriting it. Something like (minimal
pseudo-code):

     void
     alter_system_set(char *name, char *value)
     {
         /*
          * check here that the server is *not* running
          */
         ...
         ParseConfigFp(infile, AutoConfFileName, 0, LOG, &head, &tail)
         ...

         /*
          * some robust portable way of ensuring another process can't
          * modify the file(s) until we're done
          */
         lock_file(AutoConfFileName);

         replace_auto_config_value(&head, &tail, name, value);

         write_auto_conf_file(AutoConfTmpFileName, head)

         durable_rename(AutoConfTmpFileName, AutoConfFileName, ERROR);

         FreeConfigVariables(head);
         unlock_file(AutoConfFileName);
     }

I'm not sure how feasible it is to validate the provided parameter
without the server running, but if not possible, that's not any worse than the
status quo, i.e. the utility has to be trusted to write the correct parameters
to the file anyway.

The question is though - is this a change which is practical to make at this point
in the release cycle for Pg12?


Regards

Ian Barwick



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

On Wed, Jun 19, 2019 at 10:09 AM Ian Barwick
<ian.barwick@2ndquadrant.com> wrote:
>
> On 6/19/19 12:46 PM, Amit Kapila wrote:
> > On Mon, Jun 17, 2019 at 8:20 PM Ian Barwick <ian.barwick@2ndquadrant.com> wrote:
> >> On 6/15/19 1:08 AM, Stephen Frost wrote:
> >>   > * Amit Kapila (amit.kapila16@gmail.com) wrote:
> >>   >> Right.  I think if possible, it should use existing infrastructure to
> >>   >> write to postgresql.auto.conf rather than inventing a new way to
> >>   >> change it.  Apart from this issue, if we support multiple ways to edit
> >>   >> postgresql.auto.conf, we might end up with more problems like this in
> >>   >> the future where one system is not aware of the way file being edited
> >>   >> by another system.
> >>   >
> >>   > I agere that there should have been some effort put into making the way
> >>   > ALTER SYSTEM is modified be consistent between the backend and utilities
> >>   > like pg_basebackup (which would also help third party tools understand
> >>   > how a non-backend application should be modifying the file).
> >>
> >> Did you mean to say "the way postgresql.auto.conf is modified"?
> >>
> >
> > Yes, that is what we are discussing here.  I think what we can do here
> > is to extract the functionality to set the parameter in .auto.conf
> > from AlterSystemSetConfigFile and expose it via a function that takes
> > (option_name, value) as a parameter.
>
> Yup, I was just considering what's involved there, will reply to another
> mail in the thread on that.
>
> > Then we can expose it via some
> > SQL function like set_auto_config (similar to what we have now for
> > set_config/set_config_by_name).  I think if we have something like
> > that then pg_basebackup or any other utility can use it in a
> > consistent way.
>
> Umm, but the point is here, the server will *not* be running at this point,
> so calling an SQL function is out of the question. And if the server
> is running, then you just execute "ALTER SYSTEM".
>

Sure, SQL function will be a by-product of this.  Can't we expose some
function that can be used by base backup?

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

On Wed, Jun 19, 2019 at 10:27 AM Ian Barwick
<ian.barwick@2ndquadrant.com> wrote:
>
> n 6/18/19 12:41 AM, Stephen Frost wrote:
>  > Greetings,
>  >
>  > * Ian Barwick (ian.barwick@2ndquadrant.com) wrote
> (...)
>
>  >> I suggest explicitly documenting postgresql.auto.conf behaviour (and the circumstances
>  >> where it's acceptable to modify it outside of ALTER SYSTEM calls) in the documentation
>  >> (and possibly in the code), so anyone writing utilities which need to
>  >> append to postgresql.auto.conf knows what the situation is.
>  >
>  > Yeah, I would think that, ideally, we'd have some code in the common
>  > library that other utilities could leverage and which the backend would
>  > also use.
>
> So maybe something along the lines of creating a stripped-down variant of
> AlterSystemSetConfigFile() (from "src/backend/utils/misc/guc.c") which can be
> called from front-end code to safely modify .auto.conf while the server is *not*
> running.
>
> I'm not terribly familiar with the GUC code, but that would presumably mean making
> parts of the GUC parsing/handling code linkable externally (ParseConfigFp() etc.)
>

Yeah, this looks a bit tricky as we can't use ereport in the frontend
code and that is used at multiple places in that code path.

> as you'd need to parse the file before rewriting it. Something like (minimal
> pseudo-code):
>
>      void
>      alter_system_set(char *name, char *value)
>      {
>          /*
>           * check here that the server is *not* running
>           */
>          ...
>          ParseConfigFp(infile, AutoConfFileName, 0, LOG, &head, &tail)
>          ...
>
>          /*
>           * some robust portable way of ensuring another process can't
>           * modify the file(s) until we're done
>           */
>          lock_file(AutoConfFileName);
>
>          replace_auto_config_value(&head, &tail, name, value);
>
>          write_auto_conf_file(AutoConfTmpFileName, head)
>
>          durable_rename(AutoConfTmpFileName, AutoConfFileName, ERROR);
>
>          FreeConfigVariables(head);
>          unlock_file(AutoConfFileName);
>      }
>
> I'm not sure how feasible it is to validate the provided parameter
> without the server running, but if not possible, that's not any worse than the
> status quo, i.e. the utility has to be trusted to write the correct parameters
> to the file anyway.
>

Right.  I think even if someone has given wrong values, it will get
detected on next reload.

> The question is though - is this a change which is practical to make at this point
> in the release cycle for Pg12?
>

It depends on the solution/patch we come up with to solve this issue.
What is the alternative?   If we allow Alter System to remove the
duplicate entries and call the current situation good, then we are
in-a-way allowing the room for similar or more problems in the future.

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

On Mon, Jun 17, 2019 at 10:50 AM Ian Barwick
<ian.barwick@2ndquadrant.com> wrote:
> In Pg12, the code in pg_basebackup implies the correct thing to do is append to .auto.conf,
> but as demonstrated that can cause problems with duplicate entries.

Yeah.

To me, forcing every tools author to use postgresql.conf parsing tools
rather than just appending to the file is a needless burden on tool
authors.  I'd vote for just having ALTER SYSTEM silently drop all but
the last of duplicated entries.

It sounds like I might be in the minority, but I feel like the
reactions which suggest that this is somehow heresy are highly
overdone. Given that the very first time somebody wanted to do
something like this in core, they picked this approach, I think we can
assume that it is a natural approach which other people will also
attempt. There doesn't seem to be any good reason for it not to Just
Work.

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

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Mon, Jun 17, 2019 at 10:50 AM Ian Barwick
> <ian.barwick@2ndquadrant.com> wrote:
> > In Pg12, the code in pg_basebackup implies the correct thing to do is append to .auto.conf,
> > but as demonstrated that can cause problems with duplicate entries.
>
> Yeah.
>
> To me, forcing every tools author to use postgresql.conf parsing tools
> rather than just appending to the file is a needless burden on tool
> authors.  I'd vote for just having ALTER SYSTEM silently drop all but
> the last of duplicated entries.
>
> It sounds like I might be in the minority, but I feel like the
> reactions which suggest that this is somehow heresy are highly
> overdone. Given that the very first time somebody wanted to do
> something like this in core, they picked this approach, I think we can
> assume that it is a natural approach which other people will also
> attempt. There doesn't seem to be any good reason for it not to Just
> Work.

That's not quite accurate, given that it isn't how the ALTER SYSTEM call
itself works, and clearly isn't how the authors of that feature expected
things to work or they would have actually made it work.  They didn't,
and it doesn't actually work.

The notion that pg_basebackup was correct in this, when it wasn't tested
at all, evidently, even after the concern was raised, and ALTER SYSTEM
was wrong, even though it worked just fine before some later patch
started making changes to the file, based on the idea that it's the
"natural approach" doesn't make sense to me.

If the change to pg_basebackup had included a change to ALTER SYSTEM to
make it work the *same* way that pg_basebackup now does, or at least to
work with the changes that pg_basebackup were making, then maybe
everything would have been fine.

That is to say, if your recommendation is to change everything that
modifies postgresql.auto.conf to *always* append (and maybe even include
a comment about when, and who, made the change..), and to make
everything work correctly with that, then that seems like it might be a
reasonable approach (though dealing with RESETs might be a little ugly..
haven't fully thought about that).

I still don't feel that having ALTER SYSTEM just remove duplicates is a
good idea and I do think it'll lead to confusion.

Thanks,

Stephen

Robert Haas <robertmhaas@gmail.com> writes:
> To me, forcing every tools author to use postgresql.conf parsing tools
> rather than just appending to the file is a needless burden on tool
> authors.  I'd vote for just having ALTER SYSTEM silently drop all but
> the last of duplicated entries.

I haven't been paying too close attention to this thread, but isn't
that exactly what it does now and always has?  guc.c, at least, certainly
is going to interpret duplicate entries that way.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > To me, forcing every tools author to use postgresql.conf parsing tools
> > rather than just appending to the file is a needless burden on tool
> > authors.  I'd vote for just having ALTER SYSTEM silently drop all but
> > the last of duplicated entries.
>
> I haven't been paying too close attention to this thread, but isn't
> that exactly what it does now and always has?  guc.c, at least, certainly
> is going to interpret duplicate entries that way.

The issue isn't with reading them and interpreting them, it's what
happens when you run ALTER SYSTEM and it goes and modifies the file.
Presently, it basically operates on the first entry it finds when
performing a SET or a RESET.

Which also means that you can issue SET's to your heart's content, and
if there's a duplicate for that GUC, you'll never actually change what
is interpreted.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> I haven't been paying too close attention to this thread, but isn't
>> that exactly what it does now and always has?  guc.c, at least, certainly
>> is going to interpret duplicate entries that way.

> The issue isn't with reading them and interpreting them, it's what
> happens when you run ALTER SYSTEM and it goes and modifies the file.
> Presently, it basically operates on the first entry it finds when
> performing a SET or a RESET.

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.

If you had some fancy setup with comments associated with entries,
you might not be pleased with that.  But I can't muster a lot of
sympathy for tools putting comments in postgresql.auto.conf anyway;
it's not intended to be a human-readable file.

If anybody does complain, my first reaction would be to make ALTER
SYSTEM strip all comment lines too.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> I haven't been paying too close attention to this thread, but isn't
> >> that exactly what it does now and always has?  guc.c, at least, certainly
> >> is going to interpret duplicate entries that way.
>
> > The issue isn't with reading them and interpreting them, it's what
> > happens when you run ALTER SYSTEM and it goes and modifies the file.
> > Presently, it basically operates on the first entry it finds when
> > performing a SET or a RESET.
>
> 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.

Sure- and every other tool that modifies that file should know that
*that* is how you do it, and therefore, if everyone is doing it right,
you don't ever end up with duplicates in the file.  If you do, someone's
doing it wrong, and we should issue a warning.

That's more-or-less the conclusion on the other thread, as I understood
it.

> If you had some fancy setup with comments associated with entries,
> you might not be pleased with that.  But I can't muster a lot of
> sympathy for tools putting comments in postgresql.auto.conf anyway;
> it's not intended to be a human-readable file.

If we were to *keep* the duplicates, then I could see value in including
information about prior configuration entries (I mean, that's what a lot
of external tools do with our postgresql.conf file- put it into git or
some other configuration management tool...).  If we aren't keeping the
dups, then I agree that there doesn't seem much point.

> If anybody does complain, my first reaction would be to make ALTER
> SYSTEM strip all comment lines too.

Uh, I believe it already does?

Thanks,

Stephen

On Fri, Jun 21, 2019 at 8:15 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Mon, Jun 17, 2019 at 10:50 AM Ian Barwick
> <ian.barwick@2ndquadrant.com> wrote:
> > In Pg12, the code in pg_basebackup implies the correct thing to do is append to .auto.conf,
> > but as demonstrated that can cause problems with duplicate entries.
>
> Yeah.
>
> To me, forcing every tools author to use postgresql.conf parsing tools
> rather than just appending to the file is a needless burden on tool
> authors.
>

OTOH, if we give license to all the tools that they can append to the
.auto.conf file whenever they want, then, I think the contents of the
file can be unpredictable.  Basically, as of now, we allow only one
backend to write to the file, but giving a free pass to everyone can
create a problem.   This won't be a problem for pg_basebackup, but can
be for other tools.

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

On Fri, Jun 21, 2019 at 10:31 PM Stephen Frost <sfrost@snowman.net> wrote:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>
> > If anybody does complain, my first reaction would be to make ALTER
> > SYSTEM strip all comment lines too.
>
> Uh, I believe it already does?
>

Yeah, I also think so.

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

On Sun, Jun 23, 2019 at 2:43 AM Stephen Frost <sfrost@snowman.net> wrote:
>
> Greetings,
>
> On Sat, Jun 22, 2019 at 17:07 Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Fri, Jun 21, 2019 at 8:15 PM Robert Haas <robertmhaas@gmail.com> wrote:
>> >
>> > On Mon, Jun 17, 2019 at 10:50 AM Ian Barwick
>> > <ian.barwick@2ndquadrant.com> wrote:
>> > > In Pg12, the code in pg_basebackup implies the correct thing to do is append to .auto.conf,
>> > > but as demonstrated that can cause problems with duplicate entries.
>> >
>> > Yeah.
>> >
>> > To me, forcing every tools author to use postgresql.conf parsing tools
>> > rather than just appending to the file is a needless burden on tool
>> > authors.
>> >
>>
>> OTOH, if we give license to all the tools that they can append to the
>> .auto.conf file whenever they want, then, I think the contents of the
>> file can be unpredictable.  Basically, as of now, we allow only one
>> backend to write to the file, but giving a free pass to everyone can
>> create a problem.   This won't be a problem for pg_basebackup, but can
>> be for other tools.
>
>
> I don’t think anyone was suggesting that tools be allowed to modify the file while the server is running- if a change
needsto be made while the server is running, then it should be done through a call to ALTER SYSTEM. 
>
> There’s no shortage of tools that, particularly with the merger in of recovery.conf, want to modify and manipulate
thefile when the server is down though. 
>
> 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. 
>

Fair enough.   In that case, don't we need some mechanism to ensure
that if the API fails, then the old contents are retained?  Alter
system ensures that by writing first the contents to a temporary file,
but I am not sure if whatever is done by pg_basebackup has that
guarantee.

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

On Fri, Jun 21, 2019 at 11:24 AM Stephen Frost <sfrost@snowman.net> wrote:
> That's not quite accurate, given that it isn't how the ALTER SYSTEM call
> itself works, and clearly isn't how the authors of that feature expected
> things to work or they would have actually made it work.  They didn't,
> and it doesn't actually work.
>
> The notion that pg_basebackup was correct in this, when it wasn't tested
> at all, evidently, even after the concern was raised, and ALTER SYSTEM
> was wrong, even though it worked just fine before some later patch
> started making changes to the file, based on the idea that it's the
> "natural approach" doesn't make sense to me.
>
> If the change to pg_basebackup had included a change to ALTER SYSTEM to
> make it work the *same* way that pg_basebackup now does, or at least to
> work with the changes that pg_basebackup were making, then maybe
> everything would have been fine.

This argument boils down to: two people patches don't play nicely
together, and we should assume that the first patch had it right and
the second patch had it wrong, because the first patch was first.

I don't think it works like that. I think we should decide which patch
had it right by looking at what the nicest behavior actually is, not
by which one came first.  In my mind having ALTER SYSTEM drop
duplicate that other tools may have introduced is a clear winner with
basically no downside. You are arguing that it will produce confusion,
but I don't really understand who is going to be confused or why they
are going to be confused.  We can document whatever we do, and it
should be fine.  Humans aren't generally supposed to be examining this
file anyway, so they shouldn't get confused very often.

In my view, the original ALTER SYSTEM patch just has a bug -- it
doesn't modify the right copy of the setting when multiple copies are
present -- and we should just fix the bug.

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

On Fri, Jun 21, 2019 at 12:55 PM Tom Lane <tgl@sss.pgh.pa.us> 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.

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

On Sat, Jun 22, 2019 at 5:13 PM Stephen Frost <sfrost@snowman.net> 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.

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

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

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Fri, Jun 21, 2019 at 11:24 AM Stephen Frost <sfrost@snowman.net> wrote:
> > That's not quite accurate, given that it isn't how the ALTER SYSTEM call
> > itself works, and clearly isn't how the authors of that feature expected
> > things to work or they would have actually made it work.  They didn't,
> > and it doesn't actually work.
> >
> > The notion that pg_basebackup was correct in this, when it wasn't tested
> > at all, evidently, even after the concern was raised, and ALTER SYSTEM
> > was wrong, even though it worked just fine before some later patch
> > started making changes to the file, based on the idea that it's the
> > "natural approach" doesn't make sense to me.
> >
> > If the change to pg_basebackup had included a change to ALTER SYSTEM to
> > make it work the *same* way that pg_basebackup now does, or at least to
> > work with the changes that pg_basebackup were making, then maybe
> > everything would have been fine.
>
> This argument boils down to: two people patches don't play nicely
> together, and we should assume that the first patch had it right and
> the second patch had it wrong, because the first patch was first.

No, the point I was making is that one wasn't "natural" compared to the
other as we have two patches which clearly chose differently.  Had they
picked the same, as I said above, maybe everything would have been fine.

> I don't think it works like that. I think we should decide which patch
> had it right by looking at what the nicest behavior actually is, not
> by which one came first.  In my mind having ALTER SYSTEM drop
> duplicate that other tools may have introduced is a clear winner with
> basically no downside. You are arguing that it will produce confusion,
> but I don't really understand who is going to be confused or why they
> are going to be confused.  We can document whatever we do, and it
> should be fine.  Humans aren't generally supposed to be examining this
> file anyway, so they shouldn't get confused very often.

I'm not the only one who feels that it would be confusing for ALTER
SYSTEM to drop duplicates while every other tools creates them and
doesn't do anything to prevent them from being there.  As for who-
anyone who deals with PostgreSQL on a regular basis will end up running
into the "oh, huh, after pg_basebackup ran, I ended up with duplicates
in postgresql.auto.conf, I wonder if that's ok?" follow by "oh, errr, I
used to have duplicates but now they're gone?!?!  how'd that happen?",
unless, perhaps, they are reading this thread, in which case they'll
certainly know and expect it.  You can probably guess which camp is
larger.

When telling other tool authors how to manipulate PGDATA files, I really
dislike the "do as I say, not as I do" approach that you're advocating
for here.  Let's come up with a specific, clear, and ideally simple way
for everything to modify postgresql.auto.conf and let's have everything
use it.

> In my view, the original ALTER SYSTEM patch just has a bug -- it
> doesn't modify the right copy of the setting when multiple copies are
> present -- and we should just fix the bug.

Removing duplicates wouldn't be necessary for ALTER SYSTEM to just
modify the 'correct' version.

Thanks,

Stephen

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Jun 21, 2019 at 12:55 PM Tom Lane <tgl@sss.pgh.pa.us> 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

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Fri, Jun 21, 2019 at 12:55 PM Tom Lane <tgl@sss.pgh.pa.us> 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

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Sat, Jun 22, 2019 at 5:13 PM Stephen Frost <sfrost@snowman.net> wrote:
> > All that said, whatever code it is that we write for pg_basebackup to do this properly should go into our client
sidelibrary, 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

On 2019-Jun-24, Robert Haas wrote:

> On Sat, Jun 22, 2019 at 5:13 PM Stephen Frost <sfrost@snowman.net> wrote:
> > All that said, whatever code it is that we write for pg_basebackup to do this properly should go into our client
sidelibrary, 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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Stephen Frost <sfrost@snowman.net> 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

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Fri, Jun 21, 2019 at 12:55 PM Tom Lane <tgl@sss.pgh.pa.us> 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

Greetings,

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> On 2019-Jun-24, Robert Haas wrote:
>
> > On Sat, Jun 22, 2019 at 5:13 PM Stephen Frost <sfrost@snowman.net> wrote:
> > > All that said, whatever code it is that we write for pg_basebackup to do this properly should go into our client
sidelibrary, 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

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> 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

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 <sfrost@snowman.net> 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

 > 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

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

On Tue, Jun 25, 2019 at 7:31 AM Ian Barwick <ian.barwick@2ndquadrant.com> 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

On Tue, Jun 25, 2019 at 12:42 AM Stephen Frost <sfrost@snowman.net> wrote:
>
> Greetings,
>
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> > Robert Haas <robertmhaas@gmail.com> writes:
> > > On Fri, Jun 21, 2019 at 12:55 PM Tom Lane <tgl@sss.pgh.pa.us> 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

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 

Tomas Vondra <tomas.vondra@2ndquadrant.com> 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

On 8/3/19 7:27 AM, Tom Lane wrote:
> Tomas Vondra <tomas.vondra@2ndquadrant.com> 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.

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

Stephen Frost <sfrost@snowman.net> writes:
> On Fri, Aug 2, 2019 at 18:27 Tom Lane <tgl@sss.pgh.pa.us> 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

Hi,

On 2019-08-02 18:38:46 -0400, Stephen Frost wrote:
> On Fri, Aug 2, 2019 at 18:27 Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> > Tomas Vondra <tomas.vondra@2ndquadrant.com> 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

Hi,

On 2019-08-02 18:47:07 -0400, Tom Lane wrote:
> Stephen Frost <sfrost@snowman.net> 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

On Fri, Aug 02, 2019 at 06:38:46PM -0400, Stephen Frost wrote:
>Greetings,
>
>On Fri, Aug 2, 2019 at 18:27 Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>> Tomas Vondra <tomas.vondra@2ndquadrant.com> 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 

Andres Freund <andres@anarazel.de> 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

I wrote:
> Andres Freund <andres@anarazel.de> 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

On 8/3/19 8:09 AM, Tom Lane wrote:
> I wrote:
>> Andres Freund <andres@anarazel.de> 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

On 8/3/19 7:56 AM, Andres Freund wrote:
> Hi,
> 
> On 2019-08-02 18:47:07 -0400, Tom Lane wrote:
>> Stephen Frost <sfrost@snowman.net> 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

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

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

Hi,

On 2019-08-02 20:27:25 -0400, Stephen Frost wrote:
> On Fri, Aug 2, 2019 at 18:47 Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Stephen Frost <sfrost@snowman.net> writes:
> > > On Fri, Aug 2, 2019 at 18:27 Tom Lane <tgl@sss.pgh.pa.us> 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

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

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 Support, Remote DBA, Training & Services 

Tomas Vondra <tomas.vondra@2ndquadrant.com> 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

Ian Barwick <ian.barwick@2ndquadrant.com> writes:
> On 8/3/19 7:27 AM, Tom Lane wrote:
>> Tomas Vondra <tomas.vondra@2ndquadrant.com> 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;

On 8/4/19 4:13 AM, Tom Lane wrote:
> Ian Barwick <ian.barwick@2ndquadrant.com> writes:
>> On 8/3/19 7:27 AM, Tom Lane wrote:
>>> Tomas Vondra <tomas.vondra@2ndquadrant.com> 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

On 8/4/19 1:59 AM, Tom Lane wrote:> Tomas Vondra <tomas.vondra@2ndquadrant.com> 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

Greetings,

* Tomas Vondra (tomas.vondra@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 <tgl@sss.pgh.pa.us> wrote:
> >>Tomas Vondra <tomas.vondra@2ndquadrant.com> 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 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.

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.

Thanks,

Stephen

On Mon, Aug 05, 2019 at 10:21:39AM -0400, Stephen Frost wrote:
>Greetings,
>
>* Tomas Vondra (tomas.vondra@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 <tgl@sss.pgh.pa.us> wrote:
>> >>Tomas Vondra <tomas.vondra@2ndquadrant.com> 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 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.
>

I have not been following that discussion, but I acknowledge you've
raised those points before. At this point I'm really interested in this
as a RMT member, and from that position I don't quite care what happened
in November - my concern is what to do now, so that we can get 12 out.


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


regards

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

Tomas Vondra <tomas.vondra@2ndquadrant.com> 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

Isaac Morland <isaac.morland@gmail.com> 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: 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
configfileis
 
       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 <myon@debian.org>

Debian                                                        2019-07-15
PG_CONFTOOL(1)

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 <tomas.vondra@2ndquadrant.com> 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 <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

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

David Fetter <david@fetter.org> 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

On Mon, Aug 5, 2019 at 12:25 PM Tom Lane <tgl@sss.pgh.pa.us> 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

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Aug 5, 2019 at 12:25 PM Tom Lane <tgl@sss.pgh.pa.us> 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

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

On Mon, Aug 5, 2019 at 10:21 AM Stephen Frost <sfrost@snowman.net> 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

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Mon, Aug 5, 2019 at 12:25 PM Tom Lane <tgl@sss.pgh.pa.us> 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

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Mon, Aug 5, 2019 at 10:21 AM Stephen Frost <sfrost@snowman.net> 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

Greetings,

* Andres Freund (andres@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

Hi,

On 2019-08-05 13:34:39 -0400, Stephen Frost wrote:
> * Andres Freund (andres@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

On Mon, Aug 5, 2019 at 1:29 PM Stephen Frost <sfrost@snowman.net> 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

Robert Haas <robertmhaas@gmail.com> 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

On Mon, Aug 5, 2019 at 2:29 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> 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

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Aug 5, 2019 at 2:29 PM Tom Lane <tgl@sss.pgh.pa.us> 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

On Mon, Aug 5, 2019 at 2:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I don't think we need to go on about it at great length, but it seems
> to me that it'd be reasonable to point out that (a) you'd be well
> advised not to touch the file while the postmaster is up, and (b)
> last setting wins.  Those things are equally true of postgresql.conf
> of course, but I don't recall whether they're already documented.

OK, fair enough.

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

On Mon, Aug 5, 2019 at 2:35 PM Stephen Frost <sfrost@snowman.net> 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
asexternal ones, then maybe we wouldn’t be in this situation in the first place.  Clearly ALTER SYSTEM and the
pg_basebackupmodifications 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

Robert Haas <robertmhaas@gmail.com> 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

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Aug 5, 2019 at 2:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I don't think we need to go on about it at great length, but it seems
>> to me that it'd be reasonable to point out that (a) you'd be well
>> advised not to touch the file while the postmaster is up, and (b)
>> last setting wins.  Those things are equally true of postgresql.conf
>> of course, but I don't recall whether they're already documented.

> OK, fair enough.

Concretely, how about the attached?

(Digging around in config.sgml, I found that last-one-wins is stated,
but only in the context of one include file overriding another.
That's not *directly* a statement about what happens within a single
file, and it's in a different subsection anyway, so repeating the
info in 19.1.2 doesn't seem unreasonable.)

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

     <para>
@@ -185,18 +187,27 @@ shared_buffers = 128MB
      In addition to <filename>postgresql.conf</filename>,
      a <productname>PostgreSQL</productname> data directory contains a file
      <filename>postgresql.auto.conf</filename><indexterm><primary>postgresql.auto.conf</primary></indexterm>,
-     which has the same format as <filename>postgresql.conf</filename> but should
-     never be edited manually.  This file holds settings provided through
-     the <xref linkend="sql-altersystem"/> command.  This file is automatically
-     read whenever <filename>postgresql.conf</filename> is, and its settings take
-     effect in the same way.  Settings in <filename>postgresql.auto.conf</filename>
-     override those in <filename>postgresql.conf</filename>.
+     which has the same format as <filename>postgresql.conf</filename> but
+     is intended to be edited automatically not manually.  This file holds
+     settings provided through the <xref linkend="sql-altersystem"/> command.
+     This file is read whenever <filename>postgresql.conf</filename> is,
+     and its settings take effect in the same way.  Settings
+     in <filename>postgresql.auto.conf</filename> override those
+     in <filename>postgresql.conf</filename>.
+    </para>
+
+    <para>
+     External tools might also
+     modify <filename>postgresql.auto.conf</filename>, typically by appending
+     new settings to the end.  It is not recommended to do this while the
+     server is running, since a concurrent <command>ALTER SYSTEM</command>
+     command could overwrite such changes.
     </para>

     <para>
      The system view
      <link linkend="view-pg-file-settings"><structname>pg_file_settings</structname></link>
-     can be helpful for pre-testing changes to the configuration file, or for
+     can be helpful for pre-testing changes to the configuration files, or for
      diagnosing problems if a <systemitem>SIGHUP</systemitem> signal did not have the
      desired effects.
     </para>

On Mon, Aug 5, 2019 at 3:07 PM Tom Lane <tgl@sss.pgh.pa.us> 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

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Mon, Aug 5, 2019 at 2:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> I don't think we need to go on about it at great length, but it seems
> >> to me that it'd be reasonable to point out that (a) you'd be well
> >> advised not to touch the file while the postmaster is up, and (b)
> >> last setting wins.  Those things are equally true of postgresql.conf
> >> of course, but I don't recall whether they're already documented.
>
> > OK, fair enough.
>
> Concretely, how about the attached?


> (Digging around in config.sgml, I found that last-one-wins is stated,
> but only in the context of one include file overriding another.
> That's not *directly* a statement about what happens within a single
> file, and it's in a different subsection anyway, so repeating the
> info in 19.1.2 doesn't seem unreasonable.)

Agreed.

> diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
> index cdc30fa..f5986b2 100644
> --- a/doc/src/sgml/config.sgml
> +++ b/doc/src/sgml/config.sgml
> @@ -153,6 +153,8 @@ shared_buffers = 128MB
>       identifiers or numbers must be single-quoted.  To embed a single
>       quote in a parameter value, write either two quotes (preferred)
>       or backslash-quote.
> +     If the file contains multiple entries for the same parameter,
> +     all but the last one are ignored.
>      </para>

Looking at this patch quickly but also in isolation, so I could be wrong
here, but it seems like the above might be a good place to mention
"duplicate entries and comments may be removed."

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

The above hunk looks fine.

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

Alternatively, or maybe also here, we could say "note that appending to
the file as a mechanism for setting a new value by an external tool is
acceptable even though it will cause duplicates- PostgreSQL will always
use the last value set and other tools should as well.  Duplicates and
comments may be removed when rewriting the file, and parameters may be
lower-cased." (istr that last bit being true too but I haven't checked
lately).

>      <para>
>       The system view
>       <link linkend="view-pg-file-settings"><structname>pg_file_settings</structname></link>
> -     can be helpful for pre-testing changes to the configuration file, or for
> +     can be helpful for pre-testing changes to the configuration files, or for
>       diagnosing problems if a <systemitem>SIGHUP</systemitem> signal did not have the
>       desired effects.
>      </para>

This hunk looks fine.

Reviewing https://www.postgresql.org/docs/current/config-setting.html
again, it looks reasonably comprehensive regarding the format of the
file- perhaps we should link to there from the "external tools might
also modify" para..?  "Tool authors should review <link> to understand
the structure of postgresql.auto.conf".

Thanks!

Stephen

On 8/6/19 9:52 AM, Stephen Frost wrote:> Greetings,
 >
 > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
 >> Robert Haas <robertmhaas@gmail.com> writes:
 >>> On Mon, Aug 5, 2019 at 2:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
 >>>> I don't think we need to go on about it at great length, but it seems
 >>>> to me that it'd be reasonable to point out that (a) you'd be well
 >>>> advised not to touch the file while the postmaster is up, and (b)
 >>>> last setting wins.  Those things are equally true of postgresql.conf
 >>>> of course, but I don't recall whether they're already documented.
 >>
 >>> OK, fair enough.
 >>
 >> Concretely, how about the attached?
 >
 >
 >> (Digging around in config.sgml, I found that last-one-wins is stated,
 >> but only in the context of one include file overriding another.
 >> That's not *directly* a statement about what happens within a single
 >> file, and it's in a different subsection anyway, so repeating the
 >> info in 19.1.2 doesn't seem unreasonable.)
 >
 > Agreed.

+1.

 >> diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
 >> index cdc30fa..f5986b2 100644
 >> --- a/doc/src/sgml/config.sgml
 >> +++ b/doc/src/sgml/config.sgml
 >> @@ -153,6 +153,8 @@ shared_buffers = 128MB
 >>        identifiers or numbers must be single-quoted.  To embed a single
 >>        quote in a parameter value, write either two quotes (preferred)
 >>        or backslash-quote.
 >> +     If the file contains multiple entries for the same parameter,
 >> +     all but the last one are ignored.
 >>       </para>
 >
 > Looking at this patch quickly but also in isolation, so I could be wrong
 > here, but it seems like the above might be a good place to mention
 > "duplicate entries and comments may be removed."

That section applies to all configuration files, the removal is specific
to pg.auto.conf so better mentioned further down.

 >>       <para>
 >> @@ -185,18 +187,27 @@ shared_buffers = 128MB
 >>        In addition to <filename>postgresql.conf</filename>,
 >>        a <productname>PostgreSQL</productname> data directory contains a file
 >>        <filename>postgresql.auto.conf</filename><indexterm><primary>postgresql.auto.conf</primary></indexterm>,
 >> -     which has the same format as <filename>postgresql.conf</filename> but should
 >> -     never be edited manually.  This file holds settings provided through
 >> -     the <xref linkend="sql-altersystem"/> command.  This file is automatically
 >> -     read whenever <filename>postgresql.conf</filename> is, and its settings take
 >> -     effect in the same way.  Settings in <filename>postgresql.auto.conf</filename>
 >> -     override those in <filename>postgresql.conf</filename>.
 >> +     which has the same format as <filename>postgresql.conf</filename> but
 >> +     is intended to be edited automatically not manually.  This file holds
 >> +     settings provided through the <xref linkend="sql-altersystem"/> command.
 >> +     This file is read whenever <filename>postgresql.conf</filename> is,
 >> +     and its settings take effect in the same way.  Settings
 >> +     in <filename>postgresql.auto.conf</filename> override those
 >> +     in <filename>postgresql.conf</filename>.
 >> +    </para>
 >
 > The above hunk looks fine.
 >
 >> +    <para>
 >> +     External tools might also
 >> +     modify <filename>postgresql.auto.conf</filename>, typically by appending
 >> +     new settings to the end.  It is not recommended to do this while the
 >> +     server is running, since a concurrent <command>ALTER SYSTEM</command>
 >> +     command could overwrite such changes.
 >>       </para>
 >
 > Alternatively, or maybe also here, we could say "note that appending to
 > the file as a mechanism for setting a new value by an external tool is
 > acceptable even though it will cause duplicates- PostgreSQL will always
 > use the last value set and other tools should as well.  Duplicates and
 > comments may be removed when rewriting the file

FWIW, as the file is rewritten each time, *all* comments are removed
anyway (the first two comment lines in the file with the warning
are added when the new version of the file is written().

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

     <para>
      External tools which need to write configuration settings (e.g. for replication)
      where it's essential to ensure these are read last (to override versions
      of these settings present in other configuration files), may append settings to
      <filename>postgresql.auto.conf</filename>. It is not recommended to do this while
      the server is running, since a concurrent <command>ALTER SYSTEM</command>
      command could overwrite such changes. Note that a subsequent
      <command>ALTER SYSTEM</command> will cause <filename>postgresql.auto.conf</filename>,
      to be rewritten, removing any duplicate versions of the setting altered, and also
      any comment lines present.
     </para>

 >
 >>       <para>
 >>        The system view
 >>        <link linkend="view-pg-file-settings"><structname>pg_file_settings</structname></link>
 >> -     can be helpful for pre-testing changes to the configuration file, or for
 >> +     can be helpful for pre-testing changes to the configuration files, or for
 >>        diagnosing problems if a <systemitem>SIGHUP</systemitem> signal did not have the
 >>        desired effects.
 >>       </para>
 >
 > This hunk looks fine.
 >
 > Reviewing https://www.postgresql.org/docs/current/config-setting.html
 > again, it looks reasonably comprehensive regarding the format of the
 > file- perhaps we should link to there from the "external tools might
 > also modify" para..?  "Tool authors should review <link> to understand
 > the structure of postgresql.auto.conf".

This is all on the same page anyway.


Regards

Ian Barwick

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

Greetings,

* Ian Barwick (ian.barwick@2ndquadrant.com) wrote:
> On 8/6/19 9:52 AM, Stephen Frost wrote:> Greetings,
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> Robert Haas <robertmhaas@gmail.com> writes:
> >>> On Mon, Aug 5, 2019 at 2:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >>>> I don't think we need to go on about it at great length, but it seems
> >>>> to me that it'd be reasonable to point out that (a) you'd be well
> >>>> advised not to touch the file while the postmaster is up, and (b)
> >>>> last setting wins.  Those things are equally true of postgresql.conf
> >>>> of course, but I don't recall whether they're already documented.
> >>
> >>> OK, fair enough.
> >>
> >> Concretely, how about the attached?
> >
> >
> >> (Digging around in config.sgml, I found that last-one-wins is stated,
> >> but only in the context of one include file overriding another.
> >> That's not *directly* a statement about what happens within a single
> >> file, and it's in a different subsection anyway, so repeating the
> >> info in 19.1.2 doesn't seem unreasonable.)
> >
> > Agreed.
>
> +1.
>
> >> diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
> >> index cdc30fa..f5986b2 100644
> >> --- a/doc/src/sgml/config.sgml
> >> +++ b/doc/src/sgml/config.sgml
> >> @@ -153,6 +153,8 @@ shared_buffers = 128MB
> >>        identifiers or numbers must be single-quoted.  To embed a single
> >>        quote in a parameter value, write either two quotes (preferred)
> >>        or backslash-quote.
> >> +     If the file contains multiple entries for the same parameter,
> >> +     all but the last one are ignored.
> >>       </para>
> >
> > Looking at this patch quickly but also in isolation, so I could be wrong
> > here, but it seems like the above might be a good place to mention
> > "duplicate entries and comments may be removed."
>
> That section applies to all configuration files, the removal is specific
> to pg.auto.conf so better mentioned further down.

Ok, fair enough.

> >>       <para>
> >> @@ -185,18 +187,27 @@ shared_buffers = 128MB
> >>        In addition to <filename>postgresql.conf</filename>,
> >>        a <productname>PostgreSQL</productname> data directory contains a file
> >>        <filename>postgresql.auto.conf</filename><indexterm><primary>postgresql.auto.conf</primary></indexterm>,
> >> -     which has the same format as <filename>postgresql.conf</filename> but should
> >> -     never be edited manually.  This file holds settings provided through
> >> -     the <xref linkend="sql-altersystem"/> command.  This file is automatically
> >> -     read whenever <filename>postgresql.conf</filename> is, and its settings take
> >> -     effect in the same way.  Settings in <filename>postgresql.auto.conf</filename>
> >> -     override those in <filename>postgresql.conf</filename>.
> >> +     which has the same format as <filename>postgresql.conf</filename> but
> >> +     is intended to be edited automatically not manually.  This file holds
> >> +     settings provided through the <xref linkend="sql-altersystem"/> command.
> >> +     This file is read whenever <filename>postgresql.conf</filename> is,
> >> +     and its settings take effect in the same way.  Settings
> >> +     in <filename>postgresql.auto.conf</filename> override those
> >> +     in <filename>postgresql.conf</filename>.
> >> +    </para>
> >
> > The above hunk looks fine.
> >
> >> +    <para>
> >> +     External tools might also
> >> +     modify <filename>postgresql.auto.conf</filename>, typically by appending
> >> +     new settings to the end.  It is not recommended to do this while the
> >> +     server is running, since a concurrent <command>ALTER SYSTEM</command>
> >> +     command could overwrite such changes.
> >>       </para>
> >
> > Alternatively, or maybe also here, we could say "note that appending to
> > the file as a mechanism for setting a new value by an external tool is
> > acceptable even though it will cause duplicates- PostgreSQL will always
> > use the last value set and other tools should as well.  Duplicates and
> > comments may be removed when rewriting the file
>
> FWIW, as the file is rewritten each time, *all* comments are removed
> anyway (the first two comment lines in the file with the warning
> are added when the new version of the file is written().

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

> > and parameters may be
> > lower-cased." (istr that last bit being true too but I haven't checked
> > lately).
>
> Ho-hum, they won't be lower-cased, instead currently they just won't be
> overwritten if they're present in pg.auto.conf, which is a slight
> eccentricity, but not actually a problem with the current code as the new value
> will be written last. E.g.:
>
>     $ cat postgresql.auto.conf
>     # Do not edit this file manually!
>     # It will be overwritten by the ALTER SYSTEM command.
>     DEFAULT_TABLESPACE = 'space_1'
>
>     postgres=# ALTER SYSTEM SET default_tablespace ='pg_default';
>     ALTER SYSTEM
>
>     $ cat postgresql.auto.conf
>     # Do not edit this file manually!
>     # It will be overwritten by the ALTER SYSTEM command.
>     DEFAULT_TABLESPACE = 'space_1'
>     default_tablespace = 'pg_default'
>
> I don't think  that's worth worrying about now.

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

> My suggestion for the paragaph in question:
>
>     <para>
>      External tools which need to write configuration settings (e.g. for replication)
>      where it's essential to ensure these are read last (to override versions
>      of these settings present in other configuration files), may append settings to
>      <filename>postgresql.auto.conf</filename>. It is not recommended to do this while
>      the server is running, since a concurrent <command>ALTER SYSTEM</command>
>      command could overwrite such changes. Note that a subsequent
>      <command>ALTER SYSTEM</command> will cause <filename>postgresql.auto.conf</filename>,
>      to be rewritten, removing any duplicate versions of the setting altered, and also
>      any comment lines present.
>     </para>

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

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

> >>       <para>
> >>        The system view
> >>        <link linkend="view-pg-file-settings"><structname>pg_file_settings</structname></link>
> >> -     can be helpful for pre-testing changes to the configuration file, or for
> >> +     can be helpful for pre-testing changes to the configuration files, or for
> >>        diagnosing problems if a <systemitem>SIGHUP</systemitem> signal did not have the
> >>        desired effects.
> >>       </para>
> >
> > This hunk looks fine.
> >
> > Reviewing https://www.postgresql.org/docs/current/config-setting.html
> > again, it looks reasonably comprehensive regarding the format of the
> > file- perhaps we should link to there from the "external tools might
> > also modify" para..?  "Tool authors should review <link> to understand
> > the structure of postgresql.auto.conf".
>
> This is all on the same page anyway.

Ah, ok, fair enough.

Thanks!

Stephen

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

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

On 8/6/19 11:16 AM, Stephen Frost wrote:
 > Greetings,
 >
 > * Ian Barwick (ian.barwick@2ndquadrant.com) wrote:
 >> On 8/6/19 9:52 AM, Stephen Frost wrote:> Greetings,
 >>> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
 >>>>
 >>>> +    <para>
 >>>> +     External tools might also
 >>>> +     modify <filename>postgresql.auto.conf</filename>, typically by appending
 >>>> +     new settings to the end.  It is not recommended to do this while the
 >>>> +     server is running, since a concurrent <command>ALTER SYSTEM</command>
 >>>> +     command could overwrite such changes.
 >>>>        </para>
 >>>
 >>> Alternatively, or maybe also here, we could say "note that appending to
 >>> the file as a mechanism for setting a new value by an external tool is
 >>> acceptable even though it will cause duplicates- PostgreSQL will always
 >>> use the last value set and other tools should as well.  Duplicates and
 >>> comments may be removed when rewriting the file
 >>
 >> FWIW, as the file is rewritten each time, *all* comments are removed
 >> anyway (the first two comment lines in the file with the warning
 >> are added when the new version of the file is written().
 >
 > Whoah- the file is *not* rewritten each time.  It's only rewritten each
 > time by *ALTER SYSTEM*, but that it not the only thing that's modifying
 > the file.  That mistaken assumption is part of what got us into this
 > mess...

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

 >>> and parameters may be
 >>> lower-cased." (istr that last bit being true too but I haven't checked
 >>> lately).
 >>
 >> Ho-hum, they won't be lower-cased, instead currently they just won't be
 >> overwritten if they're present in pg.auto.conf, which is a slight
 >> eccentricity, but not actually a problem with the current code as the new value
 >> will be written last. E.g.:
 >>
 >>      $ cat postgresql.auto.conf
 >>      # Do not edit this file manually!
 >>      # It will be overwritten by the ALTER SYSTEM command.
 >>      DEFAULT_TABLESPACE = 'space_1'
 >>
 >>      postgres=# ALTER SYSTEM SET default_tablespace ='pg_default';
 >>      ALTER SYSTEM
 >>
 >>      $ cat postgresql.auto.conf
 >>      # Do not edit this file manually!
 >>      # It will be overwritten by the ALTER SYSTEM command.
 >>      DEFAULT_TABLESPACE = 'space_1'
 >>      default_tablespace = 'pg_default'
 >>
 >> I don't think  that's worth worrying about now.
 >
 > Erm, those are duplicates though and we're saying that ALTER SYSTEM
 > removes those...  Seems like we should be normalizing the file to be
 > consistent in this regard too.

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

Tweaked version attached.

 >> My suggestion for the paragaph in question:
 >>
 >>      <para>
 >>       External tools which need to write configuration settings (e.g. for replication)
 >>       where it's essential to ensure these are read last (to override versions
 >>       of these settings present in other configuration files), may append settings to
 >>       <filename>postgresql.auto.conf</filename>. It is not recommended to do this while
 >>       the server is running, since a concurrent <command>ALTER SYSTEM</command>
 >>       command could overwrite such changes. Note that a subsequent
 >>       <command>ALTER SYSTEM</command> will cause <filename>postgresql.auto.conf</filename>,
 >>       to be rewritten, removing any duplicate versions of the setting altered, and also
 >>       any comment lines present.
 >>      </para>
 >
 > I dislike the special-casing of ALTER SYSTEM here, where we're basically
 > saying that only ALTER SYSTEM is allowed to do this cleanup and that if
 > such cleanup is wanted then ALTER SYSTEM must be run.

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

 > What I was trying to get at is a definition of what transformations are
 > allowed and to make it clear that anything using/modifying the file
 > needs to be prepared for and work with those transformations.  I don't
 > think we want people assuming that if they don't run ALTER SYSTEM then
 > they can depend on duplicates being preserved and such..

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

 > and, yes, I
 > know that's a stretch, but if we ever want anything other than ALTER
 > SYSTEM to be able to make such changes (and I feel pretty confident that
 > we will...) then we shouldn't document things specifically about when
 > that command runs.

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


Regards

Ian Barwick


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

Ian Barwick <ian.barwick@2ndquadrant.com> writes:
> On 8/6/19 11:16 AM, Stephen Frost wrote:
>>> 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.

Good catch.

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

I feel fairly resistant to making the config.sgml explanation much longer
than what I wrote.  That chapter is material that every Postgres DBA has
to absorb, so we should *not* be burdening it with stuff that few people
need to know.

Perhaps we could put some of these details into the Notes section of the
ALTER SYSTEM ref page.  But I wonder how much of this is needed at all.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Ian Barwick <ian.barwick@2ndquadrant.com> writes:
> >>> 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.
>
> I feel fairly resistant to making the config.sgml explanation much longer
> than what I wrote.  That chapter is material that every Postgres DBA has
> to absorb, so we should *not* be burdening it with stuff that few people
> need to know.

Sure, I agree with that.

> Perhaps we could put some of these details into the Notes section of the
> ALTER SYSTEM ref page.  But I wonder how much of this is needed at all.

I'd be alright with that too, but I'd be just as fine with even a README
or something that we feel other hackers and external tool developers
would be likely to find.  I agree that all of this isn't something that
your run-of-the-mill DBA needs to know, but they are things that I'm
sure external tool authors will care about (including myself, David S,
probably the other backup/restore tool maintainers, and at least the
author of pg_conftool, presumably).

Of course, for my 2c anyway, the "low level backup API" is in the same
realm as this stuff (though it's missing important things like "what
magic exit code do you return from archive command to make PG give up
instead of retry"...) and we've got a whole ton of text in our docs
about that.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> Perhaps we could put some of these details into the Notes section of the
>> ALTER SYSTEM ref page.  But I wonder how much of this is needed at all.

> I'd be alright with that too, but I'd be just as fine with even a README
> or something that we feel other hackers and external tool developers
> would be likely to find.  I agree that all of this isn't something that
> your run-of-the-mill DBA needs to know, but they are things that I'm
> sure external tool authors will care about (including myself, David S,
> probably the other backup/restore tool maintainers, and at least the
> author of pg_conftool, presumably).

In hopes of moving this along, I've pushed Ian's last code change,
as there seems to be no real argument about that anymore.

As for the doc changes, how about the attached revision of what
I wrote previously?  It gives some passing mention to what ALTER
SYSTEM will do, without belaboring it or going into things that
are really implementation details.

As an example of the sort of implementation detail that I *don't*
want to document, I invite you to experiment with the difference
between
    ALTER SYSTEM SET TimeZone = 'America/New_York';
    ALTER SYSTEM SET "TimeZone" = 'America/New_York';

            regards, tom lane

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index cdc30fa..e99482d 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -153,6 +153,8 @@ shared_buffers = 128MB
      identifiers or numbers must be single-quoted.  To embed a single
      quote in a parameter value, write either two quotes (preferred)
      or backslash-quote.
+     If the file contains multiple entries for the same parameter,
+     all but the last one are ignored.
     </para>

     <para>
@@ -185,18 +187,29 @@ shared_buffers = 128MB
      In addition to <filename>postgresql.conf</filename>,
      a <productname>PostgreSQL</productname> data directory contains a file
      <filename>postgresql.auto.conf</filename><indexterm><primary>postgresql.auto.conf</primary></indexterm>,
-     which has the same format as <filename>postgresql.conf</filename> but should
-     never be edited manually.  This file holds settings provided through
-     the <xref linkend="sql-altersystem"/> command.  This file is automatically
-     read whenever <filename>postgresql.conf</filename> is, and its settings take
-     effect in the same way.  Settings in <filename>postgresql.auto.conf</filename>
-     override those in <filename>postgresql.conf</filename>.
+     which has the same format as <filename>postgresql.conf</filename> but
+     is intended to be edited automatically not manually.  This file holds
+     settings provided through the <xref linkend="sql-altersystem"/> command.
+     This file is read whenever <filename>postgresql.conf</filename> is,
+     and its settings take effect in the same way.  Settings
+     in <filename>postgresql.auto.conf</filename> override those
+     in <filename>postgresql.conf</filename>.
+    </para>
+
+    <para>
+     External tools may also
+     modify <filename>postgresql.auto.conf</filename>.  It is not
+     recommended to do this while the server is running, since a
+     concurrent <command>ALTER SYSTEM</command> command could overwrite
+     such changes.  Such tools might simply append new settings to the end,
+     or they might choose to remove duplicate settings and/or comments
+     (as <command>ALTER SYSTEM</command> will).
     </para>

     <para>
      The system view
      <link linkend="view-pg-file-settings"><structname>pg_file_settings</structname></link>
-     can be helpful for pre-testing changes to the configuration file, or for
+     can be helpful for pre-testing changes to the configuration files, or for
      diagnosing problems if a <systemitem>SIGHUP</systemitem> signal did not have the
      desired effects.
     </para>

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> Perhaps we could put some of these details into the Notes section of the
> >> ALTER SYSTEM ref page.  But I wonder how much of this is needed at all.
>
> > I'd be alright with that too, but I'd be just as fine with even a README
> > or something that we feel other hackers and external tool developers
> > would be likely to find.  I agree that all of this isn't something that
> > your run-of-the-mill DBA needs to know, but they are things that I'm
> > sure external tool authors will care about (including myself, David S,
> > probably the other backup/restore tool maintainers, and at least the
> > author of pg_conftool, presumably).
>
> In hopes of moving this along, I've pushed Ian's last code change,
> as there seems to be no real argument about that anymore.
>
> As for the doc changes, how about the attached revision of what
> I wrote previously?  It gives some passing mention to what ALTER
> SYSTEM will do, without belaboring it or going into things that
> are really implementation details.

It's certainly better than what we have now.

> As an example of the sort of implementation detail that I *don't*
> want to document, I invite you to experiment with the difference
> between
>     ALTER SYSTEM SET TimeZone = 'America/New_York';
>     ALTER SYSTEM SET "TimeZone" = 'America/New_York';

Implementation details and file formats / acceptable transformations
are naturally different things- a given implementation may sort things
one way or another but if there's no requirement that the file be sorted
then that's just fine and can be an implementation detail possibly based
around how duplicates are dealt with.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> In hopes of moving this along, I've pushed Ian's last code change,
>> as there seems to be no real argument about that anymore.
>> 
>> As for the doc changes, how about the attached revision of what
>> I wrote previously?  It gives some passing mention to what ALTER
>> SYSTEM will do, without belaboring it or going into things that
>> are really implementation details.

> It's certainly better than what we have now.

Hearing no other comments, I've pushed that and marked this issue closed.

            regards, tom lane

On 8/16/19 12:22 AM, Tom Lane wrote:
> Stephen Frost <sfrost@snowman.net> writes:
>> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>>> In hopes of moving this along, I've pushed Ian's last code change,
>>> as there seems to be no real argument about that anymore.
>>>
>>> As for the doc changes, how about the attached revision of what
>>> I wrote previously?  It gives some passing mention to what ALTER
>>> SYSTEM will do, without belaboring it or going into things that
>>> are really implementation details.
> 
>> It's certainly better than what we have now.
> 
> Hearing no other comments, I've pushed that and marked this issue closed.

Thanks!


Regards

Ian Barwick


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

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> +    <para>
> +     External tools may also
> +     modify <filename>postgresql.auto.conf</filename>.  It is not
> +     recommended to do this while the server is running, since a
> +     concurrent <command>ALTER SYSTEM</command> command could overwrite
> +     such changes.  Such tools might simply append new settings to the end,
> +     or they might choose to remove duplicate settings and/or comments
> +     (as <command>ALTER SYSTEM</command> will).
>      </para>

While I don't know that we necessairly have to change this langauge, I
did want to point out for folks who look at these things and consider
the challenges of this change that simply appending, when it comes to
things like backup tools and such, is just not going to work, since
you'll run into things like this:

FATAL:  multiple recovery targets specified
DETAIL:  At most one of recovery_target, recovery_target_lsn, recovery_target_name, recovery_target_time,
recovery_target_xidmay be set.
 

That's from simply doing a backup, restore with one recovery target,
then back that up and restore with a different recovery target.

Further there's the issue that if you specify a recovery target for the
first restore and then *don't* have one for the second restore, then
you'll still end up trying to restore to the first point...  So,
basically, appending just isn't actually practical for what is probably
the most common use-case these days for an external tool to go modify
postgresql.auto.conf.

And so, every backup tool author that lets a user specify a target
during the restore to generate the postgresql.auto.conf with (formerly
recovery.conf) is going to have to write enough of a parser for PG
config files to be able to find and comment or remove any recovery
target options from postgresql.auto.conf.

That'd be the kind of thing that I was really hoping we could provide a
common library for.

Thanks,

Stephen

On Wed, Aug 21, 2019 at 12:25:22PM -0400, Stephen Frost wrote:
> That'd be the kind of thing that I was really hoping we could provide a
> common library for.

Indeed.  There could be many use cases for that.  Most of the parsing
logic is in guc-file.l.  There is little dependency to elog() and
there is some handling for backend-side fd and their cleanup, but that
looks doable to me without too many ifdef FRONTEND.
--
Michael