On Thu, Jun 4, 2015 at 10:40 PM, Fujii Masao <[email protected]> wrote:
> On Mon, Jun 1, 2015 at 4:24 PM, Michael Paquier
> <[email protected]> wrote:
> > On Thu, May 28, 2015 at 9:09 PM, Michael Paquier
> > <[email protected]> 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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
