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

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Fri, Jun 21, 2019 at 11:24 AM Stephen Frost <sfrost@snowman.net> wrote:
> > That's not quite accurate, given that it isn't how the ALTER SYSTEM call
> > itself works, and clearly isn't how the authors of that feature expected
> > things to work or they would have actually made it work.  They didn't,
> > and it doesn't actually work.
> >
> > The notion that pg_basebackup was correct in this, when it wasn't tested
> > at all, evidently, even after the concern was raised, and ALTER SYSTEM
> > was wrong, even though it worked just fine before some later patch
> > started making changes to the file, based on the idea that it's the
> > "natural approach" doesn't make sense to me.
> >
> > If the change to pg_basebackup had included a change to ALTER SYSTEM to
> > make it work the *same* way that pg_basebackup now does, or at least to
> > work with the changes that pg_basebackup were making, then maybe
> > everything would have been fine.
>
> This argument boils down to: two people patches don't play nicely
> together, and we should assume that the first patch had it right and
> the second patch had it wrong, because the first patch was first.

No, the point I was making is that one wasn't "natural" compared to the
other as we have two patches which clearly chose differently.  Had they
picked the same, as I said above, maybe everything would have been fine.

> I don't think it works like that. I think we should decide which patch
> had it right by looking at what the nicest behavior actually is, not
> by which one came first.  In my mind having ALTER SYSTEM drop
> duplicate that other tools may have introduced is a clear winner with
> basically no downside. You are arguing that it will produce confusion,
> but I don't really understand who is going to be confused or why they
> are going to be confused.  We can document whatever we do, and it
> should be fine.  Humans aren't generally supposed to be examining this
> file anyway, so they shouldn't get confused very often.

I'm not the only one who feels that it would be confusing for ALTER
SYSTEM to drop duplicates while every other tools creates them and
doesn't do anything to prevent them from being there.  As for who-
anyone who deals with PostgreSQL on a regular basis will end up running
into the "oh, huh, after pg_basebackup ran, I ended up with duplicates
in postgresql.auto.conf, I wonder if that's ok?" follow by "oh, errr, I
used to have duplicates but now they're gone?!?!  how'd that happen?",
unless, perhaps, they are reading this thread, in which case they'll
certainly know and expect it.  You can probably guess which camp is
larger.

When telling other tool authors how to manipulate PGDATA files, I really
dislike the "do as I say, not as I do" approach that you're advocating
for here.  Let's come up with a specific, clear, and ideally simple way
for everything to modify postgresql.auto.conf and let's have everything
use it.

> In my view, the original ALTER SYSTEM patch just has a bug -- it
> doesn't modify the right copy of the setting when multiple copies are
> present -- and we should just fix the bug.

Removing duplicates wouldn't be necessary for ALTER SYSTEM to just
modify the 'correct' version.

Thanks,

Stephen