Re: Memory leak with XLogFileCopy since de768844 (WAL file with .partial)

From: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
To: PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org>
Cc: hlinnaka(at)iki(dot)fi
Subject: Re: Memory leak with XLogFileCopy since de768844 (WAL file with .partial)
Date: 2015-06-01 07:24:26
Message-ID: CAB7nPqTmeDxF74ao34df8wdQRsZkhzY5oKvhW5p=w=-ke2rExg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Thu, May 28, 2015 at 9:09 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)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

Attachment Content-Type Size
20150601_xlogcopy_fixes.patch text/x-patch 2.3 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Geoghegan 2015-06-01 07:29:24 Arguable RLS security bug, EvalPlanQual() paranoia
Previous Message Noah Misch 2015-06-01 04:55:34 Re: [GENERAL] 9.4.1 -> 9.4.2 problem: could not access status of transaction 1