[PATCH] remove pg_archivecleanup and pg_standby

Hi,

Am Mittwoch, den 28.10.2020, 21:44 -0500 schrieb Justin Pryzby:

Forking this thread:
/messages/by-id/fd93f1c5-7818-a02c-01e5-1075ac0d4def@iki.fi

Glancing over this in the context of pg_standby/pg_archivecleanup, I am
not sure Heikki's "Ditto" is about "remove pg_archivecleanup just like
pg_standby" or rather "keep the note until we get around doing something
with it". Probably the former, but see below.

I think these are old-fashioned since 9.6 (?), so remove them for v14.

Why 9.6?

I found it confusing when re-familiarizing myself with modern streaming
replication that there are extensions which only help do things the "old way".

I guess not many will complain about pg_standby going away, but I am
under the impression that pg_archivecleanup is still used a lot in PITR
backup environments as a handy tool to expire WAL related to expired
base backups. I certainly saw hand-assembled shell code fail with "too
many files" and things when it tried to act on large amount of WAL.

So I think the part about it being used in archive_cleanup_command can
be probably be removed, but the part about it being useful as a stand-
alone tool, in particular this part:

|In this mode, if you specify a .partial or .backup file name, then only

|the file prefix will be used as the oldestkeptwalfile. This treatment
|of .backup file name allows you to remove all WAL files archived prior
|to a specific base backup without error.

At the very least, the commit message should give a rationale on why
pg_archivebackup is retired, and what it should be replaced with, in
case valid use-cases for it are still present.

Michael

--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

On Thu, Oct 29, 2020 at 08:40:31PM +0100, Michael Banck wrote:

Am Mittwoch, den 28.10.2020, 21:44 -0500 schrieb Justin Pryzby:

Forking this thread:
/messages/by-id/fd93f1c5-7818-a02c-01e5-1075ac0d4def@iki.fi

I think these are old-fashioned since 9.6 (?), so remove them for v14.

Why 9.6?

My work doesn't currently bring me in contact with replication, so I've had to
dig through release notes. I think streaming replication was new in 9.0, and
increasingly mature throughout 9.x. Maybe someone else will say a different
release was when streaming replication became the norm and wal shipping old.

I found it confusing when re-familiarizing myself with modern streaming
replication that there are extensions which only help do things the "old way".

I guess not many will complain about pg_standby going away, but I am
under the impression that pg_archivecleanup is still used a lot in PITR
backup environments as a handy tool to expire WAL related to expired
base backups. I certainly saw hand-assembled shell code fail with "too
many files" and things when it tried to act on large amount of WAL.

I anticipate you're right, and I'll withdraw 0002.

--
Justin

On 02/11/2020 20:26, Justin Pryzby wrote:

On Thu, Oct 29, 2020 at 08:40:31PM +0100, Michael Banck wrote:

Am Mittwoch, den 28.10.2020, 21:44 -0500 schrieb Justin Pryzby:

Forking this thread:
/messages/by-id/fd93f1c5-7818-a02c-01e5-1075ac0d4def@iki.fi

I think these are old-fashioned since 9.6 (?), so remove them for v14.

Why 9.6?

My work doesn't currently bring me in contact with replication, so I've had to
dig through release notes. I think streaming replication was new in 9.0, and
increasingly mature throughout 9.x. Maybe someone else will say a different
release was when streaming replication became the norm and wal shipping old.

Removing pg_standby has been proposed a couple of times in the past. See
/messages/by-id/20170913064824.rqflkadxwpboabgw@alap3.anarazel.de
for the latest attempt.

Masao-san, back in 2014 you mentioned "fast failover" as a feature that
was missing from the built-in standby mode
(/messages/by-id/CAHGQGwEE_8vvpQk0ex6Qa_aXt-OSJ7OdZjX4uM_FtqKfxq5SbQ@mail.gmail.com).
I think that's been implemented since, with the recovery_target
settings. Would you agree?

I'm pretty sure we can remove pg_standby by now. But if there's
something crucial missing from the built-in facilities, we need to talk
about implementing them.

- Heikki

On Thu, Oct 29, 2020 at 3:40 PM Michael Banck <michael.banck@credativ.de> wrote:

I guess not many will complain about pg_standby going away, but I am
under the impression that pg_archivecleanup is still used a lot in PITR
backup environments as a handy tool to expire WAL related to expired
base backups. I certainly saw hand-assembled shell code fail with "too
many files" and things when it tried to act on large amount of WAL.

Yeah, I see pg_archivecleanup used in customer environments all the
time. Like just this morning, for example.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Nov 03, 2020 at 05:28:46PM +0200, Heikki Linnakangas wrote:

Removing pg_standby has been proposed a couple of times in the past. See /messages/by-id/20170913064824.rqflkadxwpboabgw@alap3.anarazel.de
for the latest attempt.

Masao-san, back in 2014 you mentioned "fast failover" as a feature that was
missing from the built-in standby mode (/messages/by-id/CAHGQGwEE_8vvpQk0ex6Qa_aXt-OSJ7OdZjX4uM_FtqKfxq5SbQ@mail.gmail.com).
I think that's been implemented since, with the recovery_target settings.
Would you agree?

I'm pretty sure we can remove pg_standby by now. But if there's something
crucial missing from the built-in facilities, we need to talk about
implementing them.

Reading the thread you are mentioning, it seems to me that the
statu-quo is the same, but I find rather scary that this tool is used
in exactly zero tests.

Echoing with Robert, I think that pg_archivecleanup is still useful in
many cases, so that's not something we should remove.
--
Michael

On 2020-10-29 03:44, Justin Pryzby wrote:

diff --git a/doc/src/sgml/contrib.sgml b/doc/src/sgml/contrib.sgml
index 4e833d79ef..be4292ec33 100644
--- a/doc/src/sgml/contrib.sgml
+++ b/doc/src/sgml/contrib.sgml
@@ -199,6 +199,5 @@ pages.
part of the core <productname>PostgreSQL</productname> distribution.
</para>

- &pgstandby;
</sect1>
</appendix>

With this removal, that section becomes empty. So you probably want to
clean up or reorganize this a bit.

See https://www.postgresql.org/docs/devel/contrib-prog.html for the context.

On Fri, Nov 20, 2020 at 05:26:54PM +0100, Peter Eisentraut wrote:

On 2020-10-29 03:44, Justin Pryzby wrote:

diff --git a/doc/src/sgml/contrib.sgml b/doc/src/sgml/contrib.sgml
index 4e833d79ef..be4292ec33 100644
--- a/doc/src/sgml/contrib.sgml
+++ b/doc/src/sgml/contrib.sgml
@@ -199,6 +199,5 @@ pages.
part of the core <productname>PostgreSQL</productname> distribution.
</para>
- &pgstandby;
</sect1>
</appendix>

With this removal, that section becomes empty. So you probably want to
clean up or reorganize this a bit.

See https://www.postgresql.org/docs/devel/contrib-prog.html for the context.

Oops. I guess I'd write something like this. If we just remove it, then
there'd no place to add a new server application, and "client applications"
would be the only subsection.

--
Justin

On 2020-11-21 20:41, Justin Pryzby wrote:

On Fri, Nov 20, 2020 at 05:26:54PM +0100, Peter Eisentraut wrote:

On 2020-10-29 03:44, Justin Pryzby wrote:

diff --git a/doc/src/sgml/contrib.sgml b/doc/src/sgml/contrib.sgml
index 4e833d79ef..be4292ec33 100644
--- a/doc/src/sgml/contrib.sgml
+++ b/doc/src/sgml/contrib.sgml
@@ -199,6 +199,5 @@ pages.
part of the core <productname>PostgreSQL</productname> distribution.
</para>
- &pgstandby;
</sect1>
</appendix>

With this removal, that section becomes empty. So you probably want to
clean up or reorganize this a bit.

See https://www.postgresql.org/docs/devel/contrib-prog.html for the context.

Oops. I guess I'd write something like this. If we just remove it, then
there'd no place to add a new server application, and "client applications"
would be the only subsection.

I have committed the typo fix. I don't have a well-formed opinion yet
about whether all the reservations about removing pg_standby have been
addressed.

On Wed, Nov 25, 2020 at 10:04 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

On 2020-11-21 20:41, Justin Pryzby wrote:

Oops. I guess I'd write something like this. If we just remove it, then
there'd no place to add a new server application, and "client applications"
would be the only subsection.

I have committed the typo fix. I don't have a well-formed opinion yet
about whether all the reservations about removing pg_standby have been
addressed.

I would like to commit this, because "waiting restore commands" have
confusing interactions with my proposed prefetching-during-recovery
patch[1]/messages/by-id/CA+hUKGKFeYPL9K+SRixcsx1+6HsHhqK+POZyrnnZjw1jERpGcQ@mail.gmail.com. Here's a version that fixes an error when building the docs
(there was a stray remaining <xref linkend="pgstandby"/>), and adds a
commit message. Any objections?

Furthermore, I think we should also remove the section of the manual
that describes how to write your own "waiting restore command".
Thoughts?

[1]: /messages/by-id/CA+hUKGKFeYPL9K+SRixcsx1+6HsHhqK+POZyrnnZjw1jERpGcQ@mail.gmail.com

On Wed, Jan 27, 2021 at 04:13:24PM +1300, Thomas Munro wrote:

I would like to commit this, because "waiting restore commands" have
confusing interactions with my proposed prefetching-during-recovery
patch[1]. Here's a version that fixes an error when building the docs
(there was a stray remaining <xref linkend="pgstandby"/>), and adds a
commit message. Any objections?

It looks like you are missing two references in your patch set:
$ git grep pg_standby
doc/src/sgml/high-availability.sgml: Do not use pg_standby or
similar tools with the built-in standby mode
src/backend/access/transam/xlog.c: * segment. Only recycle normal
files, pg_standby for example can create

The logic assumed in RemoveXlogFile() is actually a bit scary. I have
not checked in details but it could be possible to clean up more code
in this area?

Furthermore, I think we should also remove the section of the manual
that describes how to write your own "waiting restore command".
Thoughts?

Agreed. No objections to that.
--
Michael

On Wed, Jan 27, 2021 at 6:06 PM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Jan 27, 2021 at 04:13:24PM +1300, Thomas Munro wrote:

I would like to commit this, because "waiting restore commands" have
confusing interactions with my proposed prefetching-during-recovery
patch[1]. Here's a version that fixes an error when building the docs
(there was a stray remaining <xref linkend="pgstandby"/>), and adds a
commit message. Any objections?

It looks like you are missing two references in your patch set:
$ git grep pg_standby
doc/src/sgml/high-availability.sgml: Do not use pg_standby or
similar tools with the built-in standby mode
src/backend/access/transam/xlog.c: * segment. Only recycle normal
files, pg_standby for example can create

Thanks, fixed.

The logic assumed in RemoveXlogFile() is actually a bit scary. I have
not checked in details but it could be possible to clean up more code
in this area?

I think the check that it's a regular file is a good idea anyway, but
I removed the offending comment.

Furthermore, I think we should also remove the section of the manual
that describes how to write your own "waiting restore command".
Thoughts?

Agreed. No objections to that.

Thanks!

On 2021/01/27 14:32, Thomas Munro wrote:

On Wed, Jan 27, 2021 at 6:06 PM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Jan 27, 2021 at 04:13:24PM +1300, Thomas Munro wrote:

I would like to commit this, because "waiting restore commands" have
confusing interactions with my proposed prefetching-during-recovery
patch[1]. Here's a version that fixes an error when building the docs
(there was a stray remaining <xref linkend="pgstandby"/>), and adds a
commit message. Any objections?

I agree with this direction (i.e, remove pg_standby). BTW last month when I gave the talk about possible retire of pg_standby at PostgreSQL Unconference Tokyo, no one in audience complained about that retire.

But one question is; shouldn't we follow "usual" way to retire the feature instead of dropping that immediately? That is, mark pg_standby as obsolete, announce that pg_standby will be dropped after several releases, and then drop pg_standby. This seems safe because there might be some users. While it's been marked as obsolete, maybe WAL prefetch feature doesn't work with pg_standby, but we can live with that because it's obsolete.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Wed, Jan 27, 2021 at 05:08:56PM +0900, Fujii Masao wrote:

But one question is; shouldn't we follow "usual" way to retire the
feature instead of dropping that immediately? That is, mark
pg_standby as obsolete, announce that pg_standby will be dropped
after several releases, and then drop pg_standby. This seems safe
because there might be some users. While it's been marked as
obsolete, maybe WAL prefetch feature doesn't work with pg_standby,
but we can live with that because it's obsolete.

Thanks. FWIW, at this stage, my take is just to move on and remove
it. If we mark that as obsolete, it will stay around forever while
annoying future development.
--
Michael

On Thu, Jan 28, 2021 at 8:36 PM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Jan 27, 2021 at 05:08:56PM +0900, Fujii Masao wrote:

But one question is; shouldn't we follow "usual" way to retire the
feature instead of dropping that immediately? That is, mark
pg_standby as obsolete, announce that pg_standby will be dropped
after several releases, and then drop pg_standby. This seems safe
because there might be some users. While it's been marked as
obsolete, maybe WAL prefetch feature doesn't work with pg_standby,
but we can live with that because it's obsolete.

Thanks. FWIW, at this stage, my take is just to move on and remove
it. If we mark that as obsolete, it will stay around forever while
annoying future development.

I agree. Also, this thing is entirely separate from the server, so a
hypothetical user who really wants to upgrade to 14 but keep using
pg_standby a bit longer could always use the version that shipped with
13.

On Fri, Jan 29, 2021 at 11:13 AM Thomas Munro <thomas.munro@gmail.com> wrote:

On Thu, Jan 28, 2021 at 8:36 PM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Jan 27, 2021 at 05:08:56PM +0900, Fujii Masao wrote:

But one question is; shouldn't we follow "usual" way to retire the
feature instead of dropping that immediately? That is, mark
pg_standby as obsolete, announce that pg_standby will be dropped
after several releases, and then drop pg_standby. This seems safe
because there might be some users. While it's been marked as
obsolete, maybe WAL prefetch feature doesn't work with pg_standby,
but we can live with that because it's obsolete.

Thanks. FWIW, at this stage, my take is just to move on and remove
it. If we mark that as obsolete, it will stay around forever while
annoying future development.

I agree. Also, this thing is entirely separate from the server, so a
hypothetical user who really wants to upgrade to 14 but keep using
pg_standby a bit longer could always use the version that shipped with
13.

And, pushed.