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

On Mon, Aug 5, 2019 at 10:21 AM Stephen Frost <sfrost(at)snowman(dot)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