Thread: 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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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