Re: Continue work on changes to recovery.conf API

Greetings,

* Peter Eisentraut (peter(dot)eisentraut(at)2ndquadrant(dot)com) wrote:
> Committed with that addition.

I'm concerned that this hasn't been entirely thought through regarding
pretty common use-cases, consider a pretty typical pg_basebackup -R
usage case:

- User performs a backup with pg_basebackup -R
- Replica is then promoted to be a primary
- User performs a backup with pg_basebackup -R on the new primary
- Duplicate entries end up in postgresql.auto.conf. Ew.

Now thinking about it from the perspective of... more complete backup
solutions, which give the user the option to fill out more of what used
to go into recovery.conf, it seems like there's going to have to be a
lot more hacking around with postgresql.auto.conf, such as adding in
recovery target time and other parameters, which I'm not terribly
thrilled with.

For example, when a backup is done, typically postgresql.auto.conf comes
along with it and gets restored with it, but if that's done now without
mucking with the contents, we'll run into the same issue that
pg_basebackup has as outlined above- recovery target time and other
parameters being included in the postgresql.auto.conf, possibly multiple
times if care isn't taken to avoid that. While it used to be that you
could end up with recovery.done files ending up in backups, they were
just a nuisance and didn't impact anything.

One possible approach for dealing with at least some of these concerns
would be to remove the recovery settings from postgresql.auto.conf once
recovery is done, which is similar to how we moved recovery.conf to
recovery.done, at least. We could even create a recovery.done file if
we wanted to.

Unfortunately, this also means that users are going to have their own
issues when just using pg_basebackup and trying to perform PITR since
they'll be modifying postgresql.conf and will have to remember to remove
those settings since we aren't going to modify that. Maybe we should
have a warning or notice or something saying "hey, now that this has
been promoted, maybe you should check your config file and clear out
the recovery settings?".

Thinking about how this would work with something like pgbackrest
dropping settings into postgresql.auto.conf, users would need to be
educated to use ALTER SYSTEM if they want to move the recovery target
time to be later while doing PITR (to avoid having them hacking around
in postgresql.auto.conf), which is perhaps not terrible. What would
make that experience nicer would be a way to restart PG remotely after
making that change (well, or just making it so that users could tell PG
to continue to play forward, but as I understand it this patch doesn't
give us that).

These are my thoughts on this, at least at the moment. I've also asked
David Steele to comment, of course, since this will impact pgbackrest.

In the end, I'm not entirely convinced that eliminating recovery.conf is
actually an improvement; I think I would have rather seen the original
approach of having an 'auto' recovery.conf, but perhaps we can improve
on this since others seemed anxious to get rid of recovery.conf (though
I'm not sure why- seems like we'll likely end up with more code to
handle things cleanly with a merged recovery.auto/postgresql.auto than
if we had kept them independent).

Thanks!

Stephen