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....@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
da...@pgmasters.net
