pg_basebackup, manifests and backends older than ~12
Hi,
I have noticed that attempting to use pg_basebackup from HEAD leads to
failures when using it with backend versions from 12 and older:
$ pg_basebackup -D hoge
pg_basebackup: error: backup manifests are not supported by server
version 12beta2
pg_basebackup: removing data directory "hoge"
This is a bit backwards with what we did in the past to maintain
compatibility silently when possible, for example look at the handling
of temporary replication slots. Instead of an error when means to
force users to have to specify --no-manifest in this case, shouldn't
we silently disable the generation of the backup manifest? We know
that this option won't work on older server versions anyway.
Thanks,
--
Michael
On 4/10/20 4:09 AM, Michael Paquier wrote:
I have noticed that attempting to use pg_basebackup from HEAD leads to
failures when using it with backend versions from 12 and older:
$ pg_basebackup -D hoge
pg_basebackup: error: backup manifests are not supported by server
version 12beta2
pg_basebackup: removing data directory "hoge"This is a bit backwards with what we did in the past to maintain
compatibility silently when possible, for example look at the handling
of temporary replication slots. Instead of an error when means to
force users to have to specify --no-manifest in this case, shouldn't
we silently disable the generation of the backup manifest? We know
that this option won't work on older server versions anyway.
I'm a bit conflicted here. I see where you are coming from, but given
that writing a manifest is now the default I'm not sure silently
skipping it is ideal.
Regards,
--
-David
david@pgmasters.net
Greetings,
* David Steele (david@pgmasters.net) wrote:
On 4/10/20 4:09 AM, Michael Paquier wrote:
I have noticed that attempting to use pg_basebackup from HEAD leads to
failures when using it with backend versions from 12 and older:
$ pg_basebackup -D hoge
pg_basebackup: error: backup manifests are not supported by server
version 12beta2
pg_basebackup: removing data directory "hoge"This is a bit backwards with what we did in the past to maintain
compatibility silently when possible, for example look at the handling
of temporary replication slots. Instead of an error when means to
force users to have to specify --no-manifest in this case, shouldn't
we silently disable the generation of the backup manifest? We know
that this option won't work on older server versions anyway.I'm a bit conflicted here. I see where you are coming from, but given that
writing a manifest is now the default I'm not sure silently skipping it is
ideal.
It's only the default in v13.. Surely when we connect to a v12 or
earlier system we should just keep working and accept that we don't get
a manifest as part of that.
Thanks,
Stephen
On 4/10/20 4:41 PM, Stephen Frost wrote:
Greetings,
* David Steele (david@pgmasters.net) wrote:
On 4/10/20 4:09 AM, Michael Paquier wrote:
I have noticed that attempting to use pg_basebackup from HEAD leads to
failures when using it with backend versions from 12 and older:
$ pg_basebackup -D hoge
pg_basebackup: error: backup manifests are not supported by server
version 12beta2
pg_basebackup: removing data directory "hoge"This is a bit backwards with what we did in the past to maintain
compatibility silently when possible, for example look at the handling
of temporary replication slots. Instead of an error when means to
force users to have to specify --no-manifest in this case, shouldn't
we silently disable the generation of the backup manifest? We know
that this option won't work on older server versions anyway.I'm a bit conflicted here. I see where you are coming from, but given that
writing a manifest is now the default I'm not sure silently skipping it is
ideal.It's only the default in v13.. Surely when we connect to a v12 or
earlier system we should just keep working and accept that we don't get
a manifest as part of that.
Yeah, OK. It's certainly better than forcing the user to disable
manifests, which might also disable them for v13 clusters.
--
-David
david@pgmasters.net
Hi,
On 2020-04-10 16:32:08 -0400, David Steele wrote:
On 4/10/20 4:09 AM, Michael Paquier wrote:
I have noticed that attempting to use pg_basebackup from HEAD leads to
failures when using it with backend versions from 12 and older:
$ pg_basebackup -D hoge
pg_basebackup: error: backup manifests are not supported by server
version 12beta2
pg_basebackup: removing data directory "hoge"This is a bit backwards with what we did in the past to maintain
compatibility silently when possible, for example look at the handling
of temporary replication slots. Instead of an error when means to
force users to have to specify --no-manifest in this case, shouldn't
we silently disable the generation of the backup manifest? We know
that this option won't work on older server versions anyway.I'm a bit conflicted here. I see where you are coming from, but given that
writing a manifest is now the default I'm not sure silently skipping it is
ideal.
I think we at the very least should add a hint about how to perform a
backup without a manifest.
Greetings,
Andres Freund
On Fri, Apr 10, 2020 at 04:44:34PM -0400, David Steele wrote:
On 4/10/20 4:41 PM, Stephen Frost wrote:
It's only the default in v13.. Surely when we connect to a v12 or
earlier system we should just keep working and accept that we don't get
a manifest as part of that.Yeah, OK. It's certainly better than forcing the user to disable manifests,
which might also disable them for v13 clusters.
Exactly. My point is exactly that. The current code would force
users maintaining scripts with pg_basebackup to use --no-manifest if
such a script runs with older versions of Postgres, but we should
encourage users not do to that because we want them to use manifests
with backend versions where they are supported.
--
Michael
On Sun, Apr 12, 2020 at 08:08:17AM +0900, Michael Paquier wrote:
Exactly. My point is exactly that. The current code would force
users maintaining scripts with pg_basebackup to use --no-manifest if
such a script runs with older versions of Postgres, but we should
encourage users not do to that because we want them to use manifests
with backend versions where they are supported.
Please note that I have added an open item for this thread, and
attached is a proposal of patch. While reading the code, I have
noticed that the minimum version handling is not consistent with the
other MINIMUM_VERSION_*, so I have added one for manifests.
--
Michael
Attachments:
pgbasebackup-manifests-v1.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index de098b3558..9e5e96bd8b 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -108,6 +108,11 @@ typedef void (*WriteDataCallback) (size_t nbytes, char *buf,
*/
#define MINIMUM_VERSION_FOR_TEMP_SLOTS 100000
+/*
+ * Backup manifests are supported from version 13.
+ */
+#define MINIMUM_VERSION_FOR_MANIFESTS 130000
+
/*
* Different ways to include WAL
*/
@@ -1770,7 +1775,7 @@ BaseBackup(void)
char *basebkp;
char escaped_label[MAXPGPATH];
char *maxrate_clause = NULL;
- char *manifest_clause;
+ char *manifest_clause = NULL;
char *manifest_checksums_clause = "";
int i;
char xlogstart[64];
@@ -1836,15 +1841,6 @@ BaseBackup(void)
if (manifest)
{
- if (serverMajor < 1300)
- {
- const char *serverver = PQparameterStatus(conn, "server_version");
-
- pg_log_error("backup manifests are not supported by server version %s",
- serverver ? serverver : "'unknown'");
- exit(1);
- }
-
if (manifest_force_encode)
manifest_clause = "MANIFEST 'force-encode'";
else
@@ -1853,13 +1849,6 @@ BaseBackup(void)
manifest_checksums_clause = psprintf("MANIFEST_CHECKSUMS '%s'",
manifest_checksums);
}
- else
- {
- if (serverMajor < 1300)
- manifest_clause = "";
- else
- manifest_clause = "MANIFEST 'no'";
- }
if (verbose)
pg_log_info("initiating base backup, waiting for checkpoint to complete");
@@ -1883,7 +1872,7 @@ BaseBackup(void)
maxrate_clause ? maxrate_clause : "",
format == 't' ? "TABLESPACE_MAP" : "",
verify_checksums ? "" : "NOVERIFY_CHECKSUMS",
- manifest_clause,
+ manifest_clause ? manifest_clause : "",
manifest_checksums_clause);
if (PQsendQuery(conn, basebkp) == 0)
@@ -2589,6 +2578,10 @@ main(int argc, char **argv)
*/
umask(pg_mode_mask);
+ /* Backup manifests are supported in 13 and newer versions */
+ if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_MANIFESTS)
+ manifest = false;
+
/*
* Verify that the target directory exists, or create it. For plaintext
* backups, always require the directory. For tar backups, require it
At Mon, 13 Apr 2020 09:56:02 +0900, Michael Paquier <michael@paquier.xyz> wrote in
On Sun, Apr 12, 2020 at 08:08:17AM +0900, Michael Paquier wrote:
Exactly. My point is exactly that. The current code would force
users maintaining scripts with pg_basebackup to use --no-manifest if
such a script runs with older versions of Postgres, but we should
encourage users not do to that because we want them to use manifests
with backend versions where they are supported.Please note that I have added an open item for this thread, and
attached is a proposal of patch. While reading the code, I have
noticed that the minimum version handling is not consistent with the
other MINIMUM_VERSION_*, so I have added one for manifests.
Since I'm not sure about the work flow that contains taking a
basebackup from a server of a different version, I'm not sure which is
better between silently disabling and erroring out. However, it seems
to me, the option for replication slot is a choice of the way the tool
works which doesn't affect the result itself, but that for backup
manifest is about what the resulting backup contains. Therefore I
think it is better that pg_basebackup in PG13 should error out if the
source server doesn't support backup manifest but --no-manifest is not
specfied, and show how to accomplish their wants (, though I don't see
the wants clearly).
$ pg_basebackup ...
pg_basebackup: error: backup manifest is available from servers running PostgreSQL 13 or later
Try --no-manifest to take a backup from this server.
By the way, if I specified --manifest-checksums, it complains about
incompatible options with a message that would look strange to the
user.
pg_basebackup: error: --no-manifest and --manifest-checksums are incompatible options
("I didn't specified such an option..")
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Mon, Apr 13, 2020 at 11:52:51AM +0900, Kyotaro Horiguchi wrote:
Since I'm not sure about the work flow that contains taking a
basebackup from a server of a different version, I'm not sure which is
better between silently disabling and erroring out. However, it seems
to me, the option for replication slot is a choice of the way the tool
works which doesn't affect the result itself, but that for backup
manifest is about what the resulting backup contains. Therefore I
think it is better that pg_basebackup in PG13 should error out if the
source server doesn't support backup manifest but --no-manifest is not
specfied, and show how to accomplish their wants (, though I don't see
the wants clearly).
Not sure what Robert and other authors of the feature think about
that. What I am rather afraid of is somebody deciding to patch a
script aimed at working across multiple backend versions to add
unconditionally --no-manifest all the time, even for v13. That would
kill the purpose of encouraging the use of manifests.
By the way, if I specified --manifest-checksums, it complains about
incompatible options with a message that would look strange to the
user.pg_basebackup: error: --no-manifest and --manifest-checksums are incompatible options
("I didn't specified such an option..")
How did you trigger that? I am able to only see this failure when
using --manifest-checksums and --no-manifest together.
--
Michael
At Mon, 13 Apr 2020 13:51:07 +0900, Michael Paquier <michael@paquier.xyz> wrote in
On Mon, Apr 13, 2020 at 11:52:51AM +0900, Kyotaro Horiguchi wrote:
Since I'm not sure about the work flow that contains taking a
basebackup from a server of a different version, I'm not sure which is
better between silently disabling and erroring out. However, it seems
to me, the option for replication slot is a choice of the way the tool
works which doesn't affect the result itself, but that for backup
manifest is about what the resulting backup contains. Therefore I
think it is better that pg_basebackup in PG13 should error out if the
source server doesn't support backup manifest but --no-manifest is not
specfied, and show how to accomplish their wants (, though I don't see
the wants clearly).Not sure what Robert and other authors of the feature think about
that. What I am rather afraid of is somebody deciding to patch a
script aimed at working across multiple backend versions to add
unconditionally --no-manifest all the time, even for v13. That would
kill the purpose of encouraging the use of manifests.
I don't object that since I'm not sure about the use case of
cross-version pg_basebackup.
By the way, if I specified --manifest-checksums, it complains about
incompatible options with a message that would look strange to the
user.pg_basebackup: error: --no-manifest and --manifest-checksums are incompatible options
("I didn't specified such an option..")
How did you trigger that? I am able to only see this failure when
using --manifest-checksums and --no-manifest together.
Mmm. Sorry for the noise. I might ran unpatched version for the time.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Sun, Apr 12, 2020 at 8:56 PM Michael Paquier <michael@paquier.xyz> wrote:
On Sun, Apr 12, 2020 at 08:08:17AM +0900, Michael Paquier wrote:
Exactly. My point is exactly that. The current code would force
users maintaining scripts with pg_basebackup to use --no-manifest if
such a script runs with older versions of Postgres, but we should
encourage users not do to that because we want them to use manifests
with backend versions where they are supported.Please note that I have added an open item for this thread, and
attached is a proposal of patch. While reading the code, I have
noticed that the minimum version handling is not consistent with the
other MINIMUM_VERSION_*, so I have added one for manifests.
I think that this patch is incorrect. I have no objection to
introducing MINIMUM_VERSION_FOR_MANIFESTS, but this is not OK:
- else
- {
- if (serverMajor < 1300)
- manifest_clause = "";
- else
- manifest_clause = "MANIFEST 'no'";
- }
It seems to me that this will break --no-manifest option on v13.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Mon, Apr 13, 2020 at 11:13:06AM -0400, Robert Haas wrote:
I think that this patch is incorrect. I have no objection to
introducing MINIMUM_VERSION_FOR_MANIFESTS, but this is not OK:- else
- {
- if (serverMajor < 1300)
- manifest_clause = "";
- else
- manifest_clause = "MANIFEST 'no'";
- }It seems to me that this will break --no-manifest option on v13.
Well, the documentation tells me that as of protocol.sgml:
"For compatibility with previous releases, the default is
<literal>MANIFEST 'no'</literal>."
The code also tells me that, in line with the docs:
static void
parse_basebackup_options(List *options, basebackup_options *opt)
[...]
MemSet(opt, 0, sizeof(*opt));
opt->manifest = MANIFEST_OPTION_NO;
And there is also a TAP test for that when passing down --no-manifest,
which should not create a backup manifest:
$node->command_ok(
[
'pg_basebackup', '-D', "$tempdir/backup2", '--no-manifest',
'--waldir', "$tempdir/xlog2"
],
So, it seems to me that it is fine to remove this block, as when
--no-manifest is used, then "manifest" gets set to false, and then it
does not matter if the MANIFEST clause is added or not as we'd just
rely on the default. Keeping the block would matter if you want to
make the code more robust to a change of the default value in the
BASE_BACKUP query though, and its logic is not incorrect either. So,
if you wish to keep it, that's fine by me, but it looks cleaner to me
to remove it and more consistent with the other options like MAX_RATE,
TABLESPACE_MAP, etc.
--
Michael
On 2020-Apr-13, Michael Paquier wrote:
On Mon, Apr 13, 2020 at 11:52:51AM +0900, Kyotaro Horiguchi wrote:
Since I'm not sure about the work flow that contains taking a
basebackup from a server of a different version, I'm not sure which is
better between silently disabling and erroring out. However, it seems
to me, the option for replication slot is a choice of the way the tool
works which doesn't affect the result itself, but that for backup
manifest is about what the resulting backup contains. Therefore I
think it is better that pg_basebackup in PG13 should error out if the
source server doesn't support backup manifest but --no-manifest is not
specfied, and show how to accomplish their wants (, though I don't see
the wants clearly).Not sure what Robert and other authors of the feature think about
that. What I am rather afraid of is somebody deciding to patch a
script aimed at working across multiple backend versions to add
unconditionally --no-manifest all the time, even for v13. That would
kill the purpose of encouraging the use of manifests.
I agree, I think forcing users to specify --no-manifest when run on old
servers will cause users to write bad scripts; I vote for silently
disabling checksums.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Apr 13, 2020 at 6:26 PM Michael Paquier <michael@paquier.xyz> wrote:
Well, the documentation tells me that as of protocol.sgml:
"For compatibility with previous releases, the default is
<literal>MANIFEST 'no'</literal>."The code also tells me that, in line with the docs:
static void
parse_basebackup_options(List *options, basebackup_options *opt)
[...]
MemSet(opt, 0, sizeof(*opt));
opt->manifest = MANIFEST_OPTION_NO;And there is also a TAP test for that when passing down --no-manifest,
which should not create a backup manifest:
$node->command_ok(
[
'pg_basebackup', '-D', "$tempdir/backup2", '--no-manifest',
'--waldir', "$tempdir/xlog2"
],So, it seems to me that it is fine to remove this block, as when
--no-manifest is used, then "manifest" gets set to false, and then it
does not matter if the MANIFEST clause is added or not as we'd just
rely on the default. Keeping the block would matter if you want to
make the code more robust to a change of the default value in the
BASE_BACKUP query though, and its logic is not incorrect either. So,
if you wish to keep it, that's fine by me, but it looks cleaner to me
to remove it and more consistent with the other options like MAX_RATE,
TABLESPACE_MAP, etc.
Oh, hmm. Maybe I'm getting confused with a previous version of the
patch that behaved differently.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Mon, Apr 13, 2020 at 07:55:07PM -0400, Robert Haas wrote:
Oh, hmm. Maybe I'm getting confused with a previous version of the
patch that behaved differently.
No problem. If you prefer keeping this part of the code, that's fine
by me. If you think that the patch is suited as-is, including
silencing the error forcing to use --no-manifest on server versions
older than v13, I am fine to help out and apply it myself, but I am
also fine if you wish to take care of it by yourself.
--
Michael
On Mon, Apr 13, 2020 at 8:23 PM Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Apr 13, 2020 at 07:55:07PM -0400, Robert Haas wrote:
Oh, hmm. Maybe I'm getting confused with a previous version of the
patch that behaved differently.No problem. If you prefer keeping this part of the code, that's fine
by me. If you think that the patch is suited as-is, including
silencing the error forcing to use --no-manifest on server versions
older than v13, I am fine to help out and apply it myself, but I am
also fine if you wish to take care of it by yourself.
Feel free to go ahead.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Tue, Apr 14, 2020 at 03:13:39PM -0400, Robert Haas wrote:
Feel free to go ahead.
Thanks, let's do it then. If you have any objections about any parts
of the patch, of course please feel free.
--
Michael
On Mon, Apr 13, 2020 at 07:04:20PM -0400, Alvaro Herrera wrote:
I agree, I think forcing users to specify --no-manifest when run on old
servers will cause users to write bad scripts; I vote for silently
disabling checksums.
Okay, thanks. Are there any other opinions?
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Mon, Apr 13, 2020 at 07:04:20PM -0400, Alvaro Herrera wrote:
I agree, I think forcing users to specify --no-manifest when run on old
servers will cause users to write bad scripts; I vote for silently
disabling checksums.
Okay, thanks. Are there any other opinions?
FWIW, I concur with silently disabling the feature if the source
server can't support it.
regards, tom lane
On Tue, Apr 14, 2020 at 08:09:22PM -0400, Tom Lane wrote:
Michael Paquier <michael@paquier.xyz> writes:
On Mon, Apr 13, 2020 at 07:04:20PM -0400, Alvaro Herrera wrote:
I agree, I think forcing users to specify --no-manifest when run on old
servers will cause users to write bad scripts; I vote for silently
disabling checksums.Okay, thanks. Are there any other opinions?
FWIW, I concur with silently disabling the feature if the source
server can't support it.
Thanks. I have applied the patch, then.
--
Michael