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

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