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.
> Attached is a patch to fix the problem.
Having a second look at that, XLogFileCopy() is called only in one
place, and dstfname is never used, hence I think that it would make
sense to return unconditionally a temporary file name to the caller.
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"
Attached is a new patch.
Regards,
--
Michael
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 666fa37..9a5b350 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -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);
}
/*
@@ -5238,9 +5218,13 @@ 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);
+ tmpfname = XLogFileCopy(xlogfname, endOfLog % XLOG_SEG_SIZE);
if (!InstallXLogFileSegment(&endLogSegNo, tmpfname, false, 0, false))
- elog(ERROR, "InstallXLogFileSegment should not have failed");
+ {
+ pfree(tmpfname);
+ elog(ERROR, "could not copy partial log file");
+ }
+ 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
