Re: PATCH: Exclude additional directories in pg_basebackup
От | David Steele |
---|---|
Тема | Re: PATCH: Exclude additional directories in pg_basebackup |
Дата | |
Msg-id | ed74ee72-2fed-b6f8-a571-2bc281c751b3@pgmasters.net обсуждение исходный текст |
Ответ на | Re: PATCH: Exclude additional directories in pg_basebackup (Michael Paquier <michael.paquier@gmail.com>) |
Список | pgsql-hackers |
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
В списке pgsql-hackers по дате отправления: