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

On Thu, Jun 4, 2015 at 10:40 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:

> On Mon, Jun 1, 2015 at 4:24 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
> > 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.
>
> 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