Re: backup manifests

On 2020/04/09 23:06, Fujii Masao wrote:
>
>
> On 2020/04/09 2:35, Robert Haas wrote:
>> On Wed, Apr 8, 2020 at 1:15 AM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>>> When there is a backup_manifest in the database cluster, it's included in
>>> the backup even when --no-manifest is specified. ISTM that this is problematic
>>> because the backup_manifest is obviously not valid for the backup.
>>> So, isn't it better to always exclude the *existing* backup_manifest in the
>>> cluster from the backup, like backup_label/tablespace_map? Patch attached.
>>>
>>> Also I found the typo in the document. Patch attached.
>>
>> Both patches look good. The second one is definitely a mistake on my
>> part, and the first one seems like a totally reasonable change.
>> Thanks!
>
> Thanks for reviewing them! I pushed them.

I found other minor issues.

+ When this option is specified with a value of <literal>yes</literal>
+ or <literal>force-escape</literal>, a backup manifest is created

force-escape should be force-encode.
Patch attached.

- while ((c = getopt_long(argc, argv, "CD:F:r:RS:T:X:l:nNzZ:d:c:h:p:U:s:wWkvP",
+ while ((c = getopt_long(argc, argv, "CD:F:r:RS:T:X:l:nNzZ:d:c:h:p:U:s:wWkvPm:",

"m:" seems unnecessary, so should be removed?
Patch attached.

+ if (strcmp(basedir, "-") == 0)
+ {
+ char header[512];
+ PQExpBufferData buf;
+
+ initPQExpBuffer(&buf);
+ ReceiveBackupManifestInMemory(conn, &buf);

backup_manifest should be received only when the manifest is enabled,
so ISTM that the flag "manifest" should be checked in the above if-condition.
Thought? Patch attached.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION