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(dot)ntt(at)gmail(dot)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(at)pgmasters(dot)net