On 9/28/16 2:45 AM, Michael Paquier wrote: > On Tue, Sep 27, 2016 at 11:27 PM, David Steele <da...@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 da...@pgmasters.net -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
