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