Re: [Patch] Make pg_checksums skip foreign tablespace directories

On 2/20/20 12:55 AM, Michael Paquier wrote:
> On Wed, Feb 19, 2020 at 12:37:00PM -0600, David Steele wrote:
> 
>> As far as I can see, the pg_internal.init.XX will not be cleaned up by
>> PostgreSQL on startup.  I've only tested this in 9.6 so far, but I don't see
>> any differences in the code since then that would lead to better behavior.
>> Perhaps that's also something we should fix?
> 
> Not sure that it is worth spending cycles on that at the beginning of
> recovery as when a mapping file is written its temporary entry
> truncates any existing one present matching its name.

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.

>> I'm really not a fan of a blind prefix match.  I think we should stick with
>> only excluding files that are created by Postgres.
> 
> Thinking more on that, excluding any backup_label with a custom suffix
> worries me as it could cause a potential breakage for exiting backup
> solutions.  So attached is an updated patch making things in a
> smarter way: I have added to the exclusion lists the possibility to
> match an entry based on its prefix, or not, the choice being optional.
> This solves the problem with pg_internal.PID and is careful to not
> exclude unnecessary entries like suffixed backup labels or such.  This
> leads to some extra duplication within pg_rewind, basebackup.c and
> pg_checksums but I think we can live with that, and that makes
> back-patching simpler.  Refactoring is still tricky though as it
> relates to the use of paths across the backend and the frontend..

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.

>> So backup_label.old and
>> tablespace_map.old should just be added to the exclude list.  That's how we
>> have it in pgBackRest.
> 
> That would be a behavior change.  We could change that on HEAD, but I
> don't think that this can be back-patched as this does not cause an
> actual problem.

Right, that should be in HEAD.

> For now, my proposal is to fix the prefix first, and then let's look
> at the business with tablespaces where needed.

OK.

Regards,
-- 
-David
david@pgmasters.net