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.fujii@oss.nttdata.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