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

On Thu, May 28, 2015 at 9:09 PM, Michael Paquier
<michael.paquier@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

On Mon, Jun 1, 2015 at 4:24 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Thu, May 28, 2015 at 9:09 PM, Michael Paquier
<michael.paquier@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.

That seems to be almost harmless because the startup process will exit
just after calling XLogFileCopy(). No?

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.

+1

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.

Regards,

--
Fujii Masao

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

On Mon, Jun 1, 2015 at 4:24 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Thu, May 28, 2015 at 9:09 PM, Michael Paquier
<michael.paquier@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.

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

On Fri, Jun 5, 2015 at 12:39 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

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

On Mon, Jun 1, 2015 at 4:24 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Thu, May 28, 2015 at 9:09 PM, Michael Paquier
<michael.paquier@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.

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.

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.

Regards,

--
Fujii Masao

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

On Mon, Jun 8, 2015 at 11:52 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

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.

Thanks for updating the patch! Looks good to me. Applied.

Regards,

--
Fujii Masao

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 06/08/2015 09:04 PM, Fujii Masao wrote:

On Mon, Jun 8, 2015 at 11:52 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

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.

Thanks for updating the patch! Looks good to me. Applied.

XLogFileCopy() used to call InstallXLogFileSegment(), until I refactored
that in commit de7688442f5aaa03da60416a6aa3474738718803. That commit
added another caller of XLogFileCopy(), which didn't want to install the
segment. However, I later partially reverted that patch in commit
7cbee7c0a1db668c60c020a3fd1e3234daa562a9, and those changes to
XLogFileCopy() were not really needed anymore. I decided not to revert
those changes at that point because that refactoring seemed sensible
anyway. See my email at
/messages/by-id/555DD101.7080209@iki.fi about that.

I'm still not sure if I should've just reverted that refactoring, to
make XLogFileCopy() look the same in master and back-branches, which
makes back-patching easier, or keep the refactoring, because it makes
the code slightly nicer. But the current situation is the worst of both
worlds: the interface of XLogFileCopy() is no better than it used to be,
but it's different enough to cause merge conflicts. At this point, it's
probably best to revert the code to look the same as in 9.4.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Jun 9, 2015 at 6:25 AM, Heikki Linnakangas wrote:

I'm still not sure if I should've just reverted that refactoring, to make
XLogFileCopy() look the same in master and back-branches, which makes
back-patching easier, or keep the refactoring, because it makes the code
slightly nicer. But the current situation is the worst of both worlds: the
interface of XLogFileCopy() is no better than it used to be, but it's
different enough to cause merge conflicts. At this point, it's probably best
to revert the code to look the same as in 9.4.

That's a valid concern. What about the attached then? I think that it
is still good to keep upto to copy only data up to the switch point at
recovery exit. InstallXLogFileSegment() changes a bit as well because
of its modifications of arguments.
--
Michael

On Tue, Jun 9, 2015 at 6:25 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

On 06/08/2015 09:04 PM, Fujii Masao wrote:

On Mon, Jun 8, 2015 at 11:52 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

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.

Thanks for updating the patch! Looks good to me. Applied.

XLogFileCopy() used to call InstallXLogFileSegment(), until I refactored
that in commit de7688442f5aaa03da60416a6aa3474738718803. That commit added
another caller of XLogFileCopy(), which didn't want to install the segment.
However, I later partially reverted that patch in commit
7cbee7c0a1db668c60c020a3fd1e3234daa562a9, and those changes to
XLogFileCopy() were not really needed anymore. I decided not to revert those
changes at that point because that refactoring seemed sensible anyway. See
my email at /messages/by-id/555DD101.7080209@iki.fi
about that.

I'm still not sure if I should've just reverted that refactoring, to make
XLogFileCopy() look the same in master and back-branches, which makes
back-patching easier, or keep the refactoring, because it makes the code
slightly nicer. But the current situation is the worst of both worlds: the
interface of XLogFileCopy() is no better than it used to be, but it's
different enough to cause merge conflicts. At this point, it's probably best
to revert the code to look the same as in 9.4.

I'm not sure how much it's really nicer to keep the refactoring,
but I agree that it's better to make the code look same to make
the back-patching easier.

Regards,

--
Fujii Masao

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Jun 9, 2015 at 10:09 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Tue, Jun 9, 2015 at 6:25 AM, Heikki Linnakangas wrote:

I'm still not sure if I should've just reverted that refactoring, to make
XLogFileCopy() look the same in master and back-branches, which makes
back-patching easier, or keep the refactoring, because it makes the code
slightly nicer. But the current situation is the worst of both worlds: the
interface of XLogFileCopy() is no better than it used to be, but it's
different enough to cause merge conflicts. At this point, it's probably best
to revert the code to look the same as in 9.4.

That's a valid concern. What about the attached then? I think that it
is still good to keep upto to copy only data up to the switch point at
recovery exit. InstallXLogFileSegment() changes a bit as well because
of its modifications of arguments.

Applied. Thanks!

Regards,

--
Fujii Masao

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Jul 1, 2015 at 10:58 AM, Fujii Masao wrote:

On Tue, Jun 9, 2015 at 10:09 AM, Michael Paquier wrote:

That's a valid concern. What about the attached then? I think that it
is still good to keep upto to copy only data up to the switch point at
recovery exit. InstallXLogFileSegment() changes a bit as well because
of its modifications of arguments.

Applied. Thanks!

Thanks for showing up.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers