Re: [Patch] Make pg_checksums skip foreign tablespace directories - Mailing list pgsql-hackers

Thank you David for decrypting my previous mail.., and your
translation was correct.

At Fri, 21 Feb 2020 15:07:12 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
>  On Thu, Feb 20, 2020 at 07:37:15AM -0600, David Steele wrote:
> > But since the name includes the backend's pid you would need to get lucky
> > and have a new backend with the same pid create the file after a restart.  I
> > tried it and the old temp file was left behind after restart and first
> > connection to the database.
> > 
> > I doubt this is a big issue in the field, but it seems like it would be nice
> > to do something about it.
> 
> The natural area to do that would be around ResetUnloggedRelations().
> Still that would require combine both operations to not do any
> unnecessary lookups at the data folder paths.
> 
> > I'm not excited about the amount of code duplication between these three
> > tools.  I know this was because of back-patching various issues in the past,
> > but I really think we need to unify these data structures/functions in HEAD.
> 
> The lists are duplicated because we have never really figured out how
> to combine this code in one place.  The idea was to have all the data
> folder path logic and the lists within one header shared between the
> frontend and backend but there was not much support for that on HEAD.
> 
> >> For now, my proposal is to fix the prefix first, and then let's look
> >> at the business with tablespaces where needed.
> > 
> > OK.
> 
> I'll let this patch round for a couple of extra day, and revisit it at
> the beginning of next week.


Thank you for the version.
I didn't look it closer bat it looks in the direction I wanted.
At a quick look, the following section attracted my eyes.

+                if (strncmp(de->d_name, excludeFiles[excludeIdx].name,
+                            strlen(excludeFiles[excludeIdx].name)) == 0)
+                {
+                    elog(DEBUG1, "file \"%s\" matching prefix \"%s\" excluded from backup",
+                         de->d_name, excludeFiles[excludeIdx].name);
+                    excludeFound = true;
+                    break;
+                }
+            }
+            else
+            {
+                if (strcmp(de->d_name, excludeFiles[excludeIdx].name) == 0)
+                {
+                    elog(DEBUG1, "file \"%s\" excluded from backup", de->d_name);
+                    excludeFound = true;
+                    break;
+                }

The two str[n]cmps are different only in matching length. I don't
think we don't need to differentiate the two message there, so we
could reduce the code as:

| cmplen = strlen(excludeFiles[].name);
| if (!prefix_patch)
|   cmplen++;
| if (strncmp(d_name, excludeFilep.name, cmplen) == 0)
|   ...
  
regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center