At Fri, 31 Jan 2020 13:53:52 +0900, Michael Paquier <michael@paquier.xyz> wrote in
> On Thu, Jan 30, 2020 at 06:11:22PM +0100, Michael Banck wrote:
> > The other question is whether it is possible to end up with a
> > pg_internal.init.$PID file in a running cluster. E.g. if an instance
> > crashes and gets started up again - are those cleaned up during crash
> > recovery, or should pg_checksums ignore them? Right now pg_checksums
> > only checks against a list of filenames and only skips on exact matches
> > not prefixes so that might take a bit of work.
>
> Indeed, with a bad timing and a crash in the middle of
> write_relcache_init_file(), it could be possible to have such a file
> left around in the data folder. Having a past tablespace version left
> around after an upgrade is a pilot error in my opinion because
> pg_upgrade generates a script to cleanup past tablespaces, no? So
> your patch does not look like a good idea to me. And now that I look
> at it, if we happen to leave behind a temporary file for
> pg_internal.init, backups fail with the following error:
> 2020-01-31 13:26:18.345 JST [102076] 010_pg_basebackup.pl ERROR:
> invalid segment number 0 in file "pg_internal.init.123"
Agreed.
> So, I think that it would be better to change basebackup.c,
> pg_checksums and pg_rewind so as files are excluded if there is a
> prefix match with the exclude lists. Please see the attached.
Agreed that the tools should ignore such files. Looking excludeFile,
it seems to me that basebackup makes sure to exclude only files that
should harm. I'm not sure whether it's explicitly, but
tablespace_map.old and backup_label.old are not excluded.
The patch excludes harmless files such like "backup_label.20200131"
and the two files above.
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.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
