On Fri, Sep 27, 2019 at 8:41 PM Fujii Masao <masao.fu...@gmail.com> wrote: > > On Fri, Sep 27, 2019 at 7:16 PM Michael Paquier <mich...@paquier.xyz> wrote: > > > > On Fri, Sep 27, 2019 at 05:58:21PM +0900, Fujii Masao wrote: > > > So you think that it's better to remove them just after > > > writeTimeLineHistory()? > > > Per the following Sawada-san's comment, I was thinking that idea is > > > basically > > > not good. And, RemoveNonParentXlogFiles() also removes garbage files from > > > pg_wal. It's simpler if similar codes exist near. Thought? > > > > Sawada-san's argument of upthread is that it is not good to put > > exitArchiveRecovery() after writeTimeLineHIstory(), which is what > > cbc55da has done per the reasons mentioned in the commit log, and we > > should not change that. > > > > My argument is we know that RECOVERYXLOG and RECOVERYHISTORY are not > > needed anymore at this stage of recovery, hence we had better remove > > them as soon as possible. I am not convinced that it is a good idea > > to move the cleanup close to RemoveNonParentXlogFiles(). First, this > > is an entirely different part of the logic where the startup process > > has already switched to a new timeline. Second, we add more steps > > between the moment the two files are not needed and the moment they > > are removed, so any failure in-between would cause those files to > > still be there (we cannot say either that we will not manipulate this > > code later on) and we don't want those files to lie around. So, > > mentioning that we do the cleanup just after writeTimeLineHIstory() > > because we don't need them anymore is more consistent with what has > > been done for ages for the end of archive recovery, something that > > cbc55da unfortunately broke. > > Ok, I have no objection to remove them just after writeTimeLineHistory(). >
I abandoned once to move the removal code to between writeTimeLineHistory() and timeline switching because of expanding the window but since unlink itself will complete within a very short time it would not be problamatic much. Attached the updated patch that just moves the removal code. Regards, -- Masahiko Sawada
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 6c69eb6dd7..dcfb481f38 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -5541,17 +5541,6 @@ exitArchiveRecovery(TimeLineID endTLI, XLogRecPtr endOfLog) XLogFileName(xlogfname, ThisTimeLineID, startLogSegNo, wal_segment_size); XLogArchiveCleanup(xlogfname); - /* - * Since there might be a partial WAL segment named RECOVERYXLOG, get rid - * of it. - */ - snprintf(recoveryPath, MAXPGPATH, XLOGDIR "/RECOVERYXLOG"); - unlink(recoveryPath); /* ignore any error */ - - /* Get rid of any remaining recovered timeline-history file, too */ - snprintf(recoveryPath, MAXPGPATH, XLOGDIR "/RECOVERYHISTORY"); - unlink(recoveryPath); /* ignore any error */ - /* * Remove the signal files out of the way, so that we don't accidentally * re-enter archive recovery mode in a subsequent crash. @@ -7475,6 +7464,17 @@ StartupXLOG(void) */ writeTimeLineHistory(ThisTimeLineID, recoveryTargetTLI, EndRecPtr, reason); + + /* + * Since there might be a partial WAL segment named RECOVERYXLOG, get rid + * of it. + */ + snprintf(recoveryPath, MAXPGPATH, XLOGDIR "/RECOVERYXLOG"); + unlink(recoveryPath); /* ignore any error */ + + /* Get rid of any remaining recovered timeline-history file, too */ + snprintf(recoveryPath, MAXPGPATH, XLOGDIR "/RECOVERYHISTORY"); + unlink(recoveryPath); /* ignore any error */ } /* Save the selected TimeLineID in shared memory, too */
