On Thu, Jun 4, 2015 at 10:40 PM, Fujii Masao <masao.fu...@gmail.com> wrote:
> On Mon, Jun 1, 2015 at 4:24 PM, Michael Paquier > <michael.paqu...@gmail.com> wrote: > > On Thu, May 28, 2015 at 9:09 PM, Michael Paquier > > <michael.paqu...@gmail.com> wrote: > >> Since commit de768844, XLogFileCopy of xlog.c returns to caller a > >> pstrdup'd string that can be used afterwards for other things. > >> XLogFileCopy is used in only one place, and it happens that the result > >> string is never freed at all, leaking memory. > > That seems to be almost harmless because the startup process will exit > just after calling XLogFileCopy(). No? > Yes that's harmless. My point here is correctness, prevention does not hurt particularly if this code path is used more in the future. > Also the current error message in case of failure is very C-like: > > elog(ERROR, "InstallXLogFileSegment should not have failed"); > > I thought that we to use function names in the error messages. > > Wouldn't something like that be more adapted? > > "could not copy partial log file" or ""could not copy partial log file > > into temporary segment file" > > Or we can extend InstallXLogFileSegment so that we can give the log level > to it. When InstallXLogFileSegment is called after XLogFileCopy, we can > give ERROR as the log level and cause an exception to occur if link() or > rename() fails in InstallXLogFileSegment. > That's neat. Done this way in the attached. -- Michael
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 666fa37..5ee68c1 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -807,7 +807,7 @@ static bool XLogCheckpointNeeded(XLogSegNo new_segno); static void XLogWrite(XLogwrtRqst WriteRqst, bool flexible); static bool InstallXLogFileSegment(XLogSegNo *segno, char *tmppath, bool find_free, XLogSegNo max_segno, - bool use_lock); + bool use_lock, int elevel); static int XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli, int source, bool notexistOk); static int XLogFileReadAnyTLI(XLogSegNo segno, int emode, int source); @@ -3012,7 +3012,7 @@ XLogFileInit(XLogSegNo logsegno, bool *use_existent, bool use_lock) max_segno = logsegno + CheckPointSegments; if (!InstallXLogFileSegment(&installed_segno, tmppath, *use_existent, max_segno, - use_lock)) + use_lock, LOG)) { /* * No need for any more future segments, or InstallXLogFileSegment() @@ -3041,18 +3041,15 @@ XLogFileInit(XLogSegNo logsegno, bool *use_existent, bool use_lock) /* * Copy a WAL segment file in pg_xlog directory. * - * dstfname destination filename * srcfname source filename * upto how much of the source file to copy? (the rest is filled with * zeros) * - * If dstfname is not given, the file is created with a temporary filename, - * which is returned. Both filenames are relative to the pg_xlog directory. - * - * NB: Any existing file with the same name will be overwritten! + * The file is created with a temporary filename, which is returned. Both + * filenames are relative to the pg_xlog directory. */ static char * -XLogFileCopy(char *dstfname, char *srcfname, int upto) +XLogFileCopy(char *srcfname, int upto) { char srcpath[MAXPGPATH]; char tmppath[MAXPGPATH]; @@ -3150,25 +3147,8 @@ XLogFileCopy(char *dstfname, char *srcfname, int upto) CloseTransientFile(srcfd); - /* - * Now move the segment into place with its final name. (Or just return - * the path to the file we created, if the caller wants to handle the rest - * on its own.) - */ - if (dstfname) - { - char dstpath[MAXPGPATH]; - - snprintf(dstpath, MAXPGPATH, XLOGDIR "/%s", dstfname); - if (rename(tmppath, dstpath) < 0) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not rename file \"%s\" to \"%s\": %m", - tmppath, dstpath))); - return NULL; - } - else - return pstrdup(tmppath); + /* return name to caller, to let him hamdle the rest */ + return pstrdup(tmppath); } /* @@ -3195,6 +3175,8 @@ XLogFileCopy(char *dstfname, char *srcfname, int upto) * place. This should be TRUE except during bootstrap log creation. The * caller must *not* hold the lock at call. * + * elevel: log level used by this routine. + * * Returns TRUE if the file was installed successfully. FALSE indicates that * max_segno limit was exceeded, or an error occurred while renaming the * file into place. @@ -3202,7 +3184,7 @@ XLogFileCopy(char *dstfname, char *srcfname, int upto) static bool InstallXLogFileSegment(XLogSegNo *segno, char *tmppath, bool find_free, XLogSegNo max_segno, - bool use_lock) + bool use_lock, int elevel) { char path[MAXPGPATH]; struct stat stat_buf; @@ -3247,7 +3229,7 @@ InstallXLogFileSegment(XLogSegNo *segno, char *tmppath, { if (use_lock) LWLockRelease(ControlFileLock); - ereport(LOG, + ereport(elevel, (errcode_for_file_access(), errmsg("could not link file \"%s\" to \"%s\" (initialization of log file): %m", tmppath, path))); @@ -3259,7 +3241,7 @@ InstallXLogFileSegment(XLogSegNo *segno, char *tmppath, { if (use_lock) LWLockRelease(ControlFileLock); - ereport(LOG, + ereport(elevel, (errcode_for_file_access(), errmsg("could not rename file \"%s\" to \"%s\" (initialization of log file): %m", tmppath, path))); @@ -3748,7 +3730,7 @@ RemoveXlogFile(const char *segname, XLogRecPtr PriorRedoPtr, XLogRecPtr endptr) if (endlogSegNo <= recycleSegNo && lstat(path, &statbuf) == 0 && S_ISREG(statbuf.st_mode) && InstallXLogFileSegment(&endlogSegNo, path, - true, recycleSegNo, true)) + true, recycleSegNo, true, LOG)) { ereport(DEBUG2, (errmsg("recycled transaction log file \"%s\"", @@ -5238,9 +5220,10 @@ exitArchiveRecovery(TimeLineID endTLI, XLogRecPtr endOfLog) * considerations. But we should be just as tense as XLogFileInit to * avoid emplacing a bogus file. */ - tmpfname = XLogFileCopy(NULL, xlogfname, endOfLog % XLOG_SEG_SIZE); - if (!InstallXLogFileSegment(&endLogSegNo, tmpfname, false, 0, false)) - elog(ERROR, "InstallXLogFileSegment should not have failed"); + tmpfname = XLogFileCopy(xlogfname, endOfLog % XLOG_SEG_SIZE); + (void) InstallXLogFileSegment(&endLogSegNo, tmpfname, false, + 0, false, ERROR); + pfree(tmpfname); } else {
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers