Re: PATCH: Exclude additional directories in pg_basebackup

On 9/28/16 2:45 AM, Michael Paquier wrote:
> 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 :)

Oops, this was intended to mean that I changed:

>>> +const char *excludeFile[] =
>>> excludeFiles[]?

> + <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.

OK, I agree that's confusing and probably not helpful to the user.

> + /* 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.

OK.

> + /* 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.

OK.

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

OK.

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

OK.

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

That was my thinking as well. It's up to Peter, though.

> 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

I think setting excludeFound = false the second time is redundant since
it must be false at that point. Am I missing something or are you just
being cautious in case code changes above?

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

The v6 patch looks good to me.

--
-David
david(at)pgmasters(dot)net