On 3/8/19 6:08 AM, Magnus Hagander wrote:
On Thu, Mar 7, 2019 at 5:35 PM Michael Paquier <mich...@paquier.xyz
<mailto:mich...@paquier.xyz>> wrote:
On Thu, Mar 07, 2019 at 11:33:20AM +0200, David Steele wrote:
> OK, here's a new version that splits the deprecation notes from the
> discussion of risks. I also fixed the indentation.
The documentation part looks fine to me. Just one nit regarding the
error hint.
> - errhint("If you are not restoring from a backup, try
removing the file \"%s/backup_label\".", DataDir)));
> + errhint("If you are restoring from a backup, touch
\"%s/recovery.signal\" and add recovery options to
\"%s/postgresql.auto.conf\".\n"
Here do we really want to recommend adding options to
postgresql.auto.conf? This depends a lot on the solution integration
so I think that this hint could actually confuse some users because it
implies that they kind of *have* to do so, which is not correct. I
would recommend to be a bit more generic and just use "and add
necessary recovery configuration".
Agreed, I think we should never tell people to "add recovery options to
postgresql.auto.conf". Becuase they should never do that manually. If we
want to suggest people use postgresql.auto.conf surely they should be
using ALTER SYSTEM SET. Which of course doesn't work in this case, since
postgrsql isn't running yet.
So yeah either that, or say "add to postgresql.conf" without the auto?
I went with Michael's suggestion. Attached is a new patch.
I also think we should set a flag and throw the error below this if/else
block. This is a rather large message and maintaining two copies of it
is not ideal.
Please note that there have been objections to the patch later in this
thread by Peter and Robert. I'm not very interested in watering down
the documentation changes as Peter suggests, but I think at the very
least we should commit the added hints in the error message. For many
users this error will be their first point of contact with the
backup_label issue/behavior.
Regards,
--
-David
da...@pgmasters.net
From 87a53d0dc0e2118c553110813209eba55174bb1e Mon Sep 17 00:00:00 2001
From: David Steele <da...@pgmasters.net>
Date: Wed, 20 Mar 2019 16:16:37 +0400
Subject: [PATCH] Add exclusive backup deprecation notes to documentation.
Update the exclusive backup documentation to explain the limitations of
the exclusive backup mode and make it clear that the feature is
deprecated.
Update the log message when the backup_label is found but recovery
cannot proceed.
---
doc/src/sgml/backup.sgml | 46 +++++++++++++++++++++++++++++++
src/backend/access/transam/xlog.c | 10 +++++--
2 files changed, 54 insertions(+), 2 deletions(-)
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index a73fd4d044..ab5c81d382 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -948,6 +948,17 @@ SELECT * FROM pg_stop_backup(false, true);
</sect3>
<sect3 id="backup-lowlevel-base-backup-exclusive">
<title>Making an exclusive low level backup</title>
+
+ <note>
+ <para>
+ The exclusive backup method is deprecated and should be avoided in favor
+ of the non-exclusive backup method, e.g.
+ <application>pg_basebackup</application>. This method is deprecated
+ because it has serious risks which are enumerated at the end of this
+ section.
+ </para>
+ </note>
+
<para>
The process for an exclusive backup is mostly the same as for a
non-exclusive one, but it differs in a few key steps. This type of backup
@@ -1054,6 +1065,41 @@ SELECT pg_stop_backup();
</listitem>
</orderedlist>
</para>
+
+ <note>
+ <para>
+ The primary issue with the exclusive method is that the
+ <filename>backup_label</filename> file is written into the data directory
+ when <function>pg_start_backup</function> is called and remains until
+ <function>pg_stop_backup</function> is called. If
+ <productname>PostgreSQL</productname> or the host terminates abnormally,
+ then <filename>backup_label</filename> will be left in the data directory
+ and <productname>PostgreSQL</productname> probably will not start. A log
+ message recommends that <filename>backup_label</filename> be removed if
+ not restoring from a backup.
+ </para>
+
+ <para>
+ However, if <filename>backup_label</filename> is present because a
restore
+ is actually in progress, then removing it will result in corruption. For
+ this reason it is not recommended to automate the removal of
+ <filename>backup_label</filename>.
+ </para>
+
+ <para>
+ Another issue with exclusive backup mode is that it will continue until
+ <function>pg_stop_backup</function> is called, even if the calling
process
+ is no longer performing the backup. The next time
+ <function>pg_start_backup</function> is called it will fail unless
+ <function>pg_stop_backup</function> is called manually first.
+ </para>
+
+ <para>
+ Finally, only one exclusive backup may be run at a time. However, it is
+ possible to run an exclusive backup at the same time as any number of
+ non-exclusive backups.
+ </para>
+ </note>
</sect3>
<sect3 id="backup-lowlevel-base-backup-data">
<title>Backing up the data directory</title>
diff --git a/src/backend/access/transam/xlog.c
b/src/backend/access/transam/xlog.c
index ad12ebc426..3bb1e406e1 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6365,14 +6365,20 @@ StartupXLOG(void)
if (!ReadRecord(xlogreader, checkPoint.redo,
LOG, false))
ereport(FATAL,
(errmsg("could not find
redo location referenced by checkpoint record"),
- errhint("If you are
not restoring from a backup, try removing the file \"%s/backup_label\".",
DataDir)));
+ errhint("If you are
restoring from a backup, touch \"%s/recovery.signal\" and add required recovery
options.\n"
+ "If you are not
restoring from a backup, try removing the file \"%s/backup_label\".\n"
+ "Be careful: removing
\"%s/backup_label\" will result in a corrupt cluster if restoring from a
backup.",
+ DataDir, DataDir,
DataDir)));
}
}
else
{
ereport(FATAL,
(errmsg("could not locate required
checkpoint record"),
- errhint("If you are not restoring from
a backup, try removing the file \"%s/backup_label\".", DataDir)));
+ errhint("If you are restoring from a
backup, touch \"%s/recovery.signal\" and add required recovery options.\n"
+ "If you are not
restoring from a backup, try removing the file \"%s/backup_label\".\n"
+ "Be careful: removing
\"%s/backup_label\" will result in a corrupt cluster if restoring from a
backup.",
+ DataDir, DataDir,
DataDir)));
wasShutdown = false; /* keep compiler quiet */
}
--
2.17.2 (Apple Git-113)
