On 9/28/16 2:45 AM, Michael Paquier wrote:
> On Tue, Sep 27, 2016 at 11:27 PM, David Steele <david@pgmasters.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@pgmasters.net