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

Greetings,

On Fri, Aug 2, 2019 at 20:46 Andres Freund <andres(at)anarazel(dot)de> wrote:

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

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.

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

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.

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.

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

Thanks,

Stephen