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

Greetings,

On Mon, Aug 5, 2019 at 14:38 Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

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

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

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

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

Thanks,

Stephen