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

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