Re: [Patch] Make pg_checksums skip foreign tablespace directories

On 2/19/20 2:13 AM, Michael Paquier wrote:
> On Fri, Jan 31, 2020 at 05:39:36PM +0900, Kyotaro Horiguchi wrote:
>> At Fri, 31 Jan 2020 17:30:43 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
>>> I don't think that is a problem right away, of course. It looks good
>>> to me except for the possible excessive exclusion.  So, I don't object
>>> it if we don't mind that.
>>
>> That's a bit wrong.  All the discussion is only on excludeFiles.  I
>> think we should refrain from letting more files match to
>> nohecksumFiles.
> 
> I am not sure what you are saying here.  Are you saying that we should
> not use a prefix matching for that part?  Or are you saying that we
> should not touch this list at all?

Perhaps he is saying that if it is already excluded it won't be 
checksummed.  So, if pg_internal.init* is excluded from the backup, that 
is all that is needed.  If so, I agree.  This might not help 
pg_verify_checksums, though, except that it should be applying the same 
rules.

> Please note that pg_internal.init is listed within noChecksumFiles in
> basebackup.c, so we would miss any temporary pg_internal.init.PID if
> we don't check after the file prefix and the base backup would issue
> extra WARNING messages, potentially masking messages that could
> matter.  So let's fix that as well.

Agreed.  Though, I think pg_internal.init.XX should be excluded from the 
backup as well.

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?

> I agree that a side effect of this change would be to discard anything
> prefixed with "backup_label" or "tablespace_map", including any old,
> renamed files.  Do you know of any backup solutions which could be
> impacted by that?  I am adding David Steele and Stephen Frost in CC so
> as they can comment based on their experience in this area.  I recall
> that backrest stuff uses the replication protocol, but I may be
> wrong.

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.  So 
backup_label.old and tablespace_map.old should just be added to the 
exclude list.  That's how we have it in pgBackRest.

Regards,
-- 
-David
david@pgmasters.net