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
