On Fri, Jun 5, 2015 at 10:45 PM, Fujii Masao wrote:
> Why don't we call InstallXLogFileSegment() at the end of XLogFileCopy()?
> If we do that, the risk of memory leak you're worried will disappear at all.
Yes, that looks fine, XLogFileCopy() would copy to a temporary file,
then install it definitely. Updated patch attached.
--
Michael
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 666fa37..c03f70e 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,16 @@ 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)
+ * segno identify segment to install.
*
- * 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 first copied with a temporary filename, and then installed as
+ * a newly-created segment.
*/
-static char *
-XLogFileCopy(char *dstfname, char *srcfname, int upto)
+static void
+XLogFileCopy(char *srcfname, int upto, XLogSegNo segno)
{
char srcpath[MAXPGPATH];
char tmppath[MAXPGPATH];
@@ -3150,25 +3148,9 @@ 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);
+ /* install the new file */
+ (void) InstallXLogFileSegment(&segno, tmppath, false,
+ 0, false, ERROR);
}
/*
@@ -3195,6 +3177,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 +3186,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 +3231,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 +3243,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 +3732,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\"",
@@ -5227,8 +5211,6 @@ exitArchiveRecovery(TimeLineID endTLI, XLogRecPtr endOfLog)
*/
if (endLogSegNo == startLogSegNo)
{
- char *tmpfname;
-
XLogFileName(xlogfname, endTLI, endLogSegNo);
/*
@@ -5238,9 +5220,7 @@ 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");
+ XLogFileCopy(xlogfname, endOfLog % XLOG_SEG_SIZE, endLogSegNo);
}
else
{
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
