From: | David Steele <david(at)pgmasters(dot)net> |
---|---|
To: | Michael Paquier <michael(at)paquier(dot)xyz> |
Cc: | Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, michael(dot)banck(at)credativ(dot)de, pgsql-hackers(at)postgresql(dot)org, Stephen Frost <sfrost(at)snowman(dot)net> |
Subject: | Re: [Patch] Make pg_checksums skip foreign tablespace directories |
Date: | 2020-02-20 13:37:15 |
Message-ID: | 2e9975f1-36bd-b7e8-ea13-5b196030c74c@pgmasters.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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(at)pgmasters(dot)net
From | Date | Subject | |
---|---|---|---|
Next Message | Julien Rouhaud | 2020-02-20 13:52:29 | Re: Minor improvement to partition_bounds_copy() |
Previous Message | Tomas Vondra | 2020-02-20 13:36:02 | Re: Parallel copy |