Re: PATCH: Exclude additional directories in pg_basebackup

On Tue, Sep 27, 2016 at 11:27 PM, David Steele <david(at)pgmasters(dot)net> wrote:
> On 9/26/16 2:36 AM, Michael Paquier wrote:
>
>> Just a nit:
>> <para>
>> - <filename>postmaster.pid</>
>> + <filename>postmaster.pid</> and <filename>postmaster.opts</>
>> </para>
>> Having one <para> block for each file would be better.
>
> OK, changed.

You did not actually change it :)

>> +const char *excludeFile[] =
>> excludeFiles[]?
>>
>> +# Move pg_replslot out of $pgdata and create a symlink to it
>> +rename("$pgdata/pg_replslot", "$tempdir/pg_replslot")
>> + or die "unable to move $pgdata/pg_replslot";
>> +symlink("$tempdir/pg_replslot", "$pgdata/pg_replslot");
>> This will blow up on Windows. Those tests need to be included in the
>> SKIP block. Even if your code needs to support junction points on
>> Windows, as perl does not offer an equivalent for it we just cannot
>> test it on this platform.
>
> Fixed.

Thanks for the updated version.

+ <para>
+ <filename>backup_label</> and <filename>tablespace_map</>. If these
+ files exist they belong to an exclusive backup and are not applicable
+ to the base backup.
+ </para>
This is a bit confusing. When using pg_basebackup the files are
ignored, right, but they are included in the tar stream so they are
not excluded at the end. So we had better remove purely this
paragraph. Similarly, postgresql.auto.conf.tmp is something that
exists only for a short time frame. Not listing it directly is fine
IMO.

+ /* If symlink, write it as a directory anyway */
+#ifndef WIN32
+ if (S_ISLNK(statbuf->st_mode))
+#else
+ if (pgwin32_is_junction(pathbuf))
+#endif
+
+ statbuf->st_mode = S_IFDIR | S_IRWXU;
Indentation here is confusing. Coverity would complain.

+ /* Stat the file */
+ snprintf(pathbuf, MAXPGPATH, "%s/%s", path, de->d_name);
There is no need to put the stat() call this early in the loop as this
is just used for re-creating empty directories.

+ if (strcmp(pathbuf + basepathlen + 1,
+ excludeFiles[excludeIdx]) == 0)
There is no need for complicated maths, you can just use de->d_name.

pg_xlog has somewhat a similar treatment, but per the exception
handling of archive_status it is better to leave its check as-is.

The DEBUG1 entries are really useful for debugging, it would be nice
to keep them in the final patch.

Thinking harder, your logic can be simplified. You could just do the following:
- Check for interrupts first
- Look at the list of excluded files
- Call lstat()
- Check for excluded directories

After all that fixed, I have moved the patch to "Ready for Committer".
Please use the updated patch though.
--
Michael