Thread: InsertXLogFile in pg_resetxlog

InsertXLogFile in pg_resetxlog

From
Martijn van Oosterhout
Date:
There's been some new code added to pg_resetxlog which is confusing
enough that Coverity is convinced there's a possible memory leak in
InsertXLogFile. I think it may actually be a bug. At the least this bit
needs rewriting to make it clearer what it does.

What I think happens is this:

1. Assume the xlogfilelist has more than two entries already
2. In the loop CmpXLogFileOT returns true the first time, false the
second

At this point Prev = xlogfilelist and Curr = xlogfilelist->next and
append2end = false. With these conditions all if tests fail and the
file is never linked into the list.

May I propose the entire part of that function after the comment /* the
list is empty. */ be replaced with something like the following (or
whatever idiom people prefer for singly-linked lists):

--- cut ---
/* currp points to memory location where the pointer needs to be updated */
XLogFileName **currp = &xlogfilelist;

while( *currp && CmpXLogFileOT( NewSegFile, *currp ) )currp = &( (*currp)->next );

NewSegFile->next = *currp;
*currp = NewSegFile;
--- cut ---

Have a nice day,
--
Martijn van Oosterhout   <kleptog@svana.org>   http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

Re: InsertXLogFile in pg_resetxlog

From
Tom Lane
Date:
Martijn van Oosterhout <kleptog@svana.org> writes:
> May I propose the entire part of that function after the comment /* the
> list is empty. */ be replaced with something like the following (or
> whatever idiom people prefer for singly-linked lists):

This certainly looks like it was written by someone who'd just learned
about lists yesterday :-(.  I wonder how many other problems there are
in that resetxlog patch?  I didn't bother to look at it at all myself.
Anyone have time to review it?

http://archives.postgresql.org/pgsql-committers/2006-04/msg00299.php
        regards, tom lane


Re: InsertXLogFile in pg_resetxlog

From
"Jonah H. Harris"
Date:
On 5/1/06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> This certainly looks like it was written by someone who'd just learned
> about lists yesterday :-(.  I wonder how many other problems there are
> in that resetxlog patch?  I didn't bother to look at it at all myself.
> Anyone have time to review it?

I just scanned it, and it's pretty ugly overall.  Did one of you guys
want to clean it up?  If not, I'll do it today.


--
Jonah H. Harris, Database Internals Architect
EnterpriseDB Corporation
732.331.1324


Re: InsertXLogFile in pg_resetxlog

From
"Jonah H. Harris"
Date:
On 5/1/06, Jonah H. Harris <jonah.harris@gmail.com> wrote:
> I just scanned it, and it's pretty ugly overall.  Did one of you guys
> want to clean it up?  If not, I'll do it today.

While refactoring the patch, I've noticed that this patch allowed
pg_resetxlog to proceed while the server could potentially be up... is
this the desired behavior or should we require the lock file to be
removed first (as it was prior to this patch)?

--
Jonah H. Harris, Database Internals Architect
EnterpriseDB Corporation
732.331.1324


Re: InsertXLogFile in pg_resetxlog

From
Tom Lane
Date:
"Jonah H. Harris" <jonah.harris@gmail.com> writes:
> While refactoring the patch, I've noticed that this patch allowed
> pg_resetxlog to proceed while the server could potentially be up... is
> this the desired behavior or should we require the lock file to be
> removed first (as it was prior to this patch)?

Definitely bad, very bad.  Please put back the lock-checking code.
        regards, tom lane


Re: InsertXLogFile in pg_resetxlog

From
"Jonah H. Harris"
Date:
On 5/1/06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Definitely bad, very bad.  Please put back the lock-checking code.

That's what I was thinking.

--
Jonah H. Harris, Database Internals Architect
EnterpriseDB Corporation
732.331.1324


Re: InsertXLogFile in pg_resetxlog

From
"Jonah H. Harris"
Date:
Just to update everyone, I've refactored a good amount of the
rebuild-control-values-from-WAL code and should have it ready for
-patches tomorrow.

--
Jonah H. Harris, Database Internals Architect
EnterpriseDB Corporation
732.331.1324


Re: InsertXLogFile in pg_resetxlog

From
Martijn van Oosterhout
Date:
On Mon, May 01, 2006 at 10:26:33PM -0400, Jonah H. Harris wrote:
> Just to update everyone, I've refactored a good amount of the
> rebuild-control-values-from-WAL code and should have it ready for
> -patches tomorrow.

I've not seen any patch for this come past...

Have a nice day,
--
Martijn van Oosterhout   <kleptog@svana.org>   http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

Re: InsertXLogFile in pg_resetxlog

From
"Jonah H. Harris"
Date:
On 5/6/06, Martijn van Oosterhout <kleptog@svana.org> wrote:
> I've not seen any patch for this come past...

Yes, I got a little busy.  I ended up refactoring a good amount of the
code because the entire thing is a little ugly.  I'll go ahead and
just fix the Coverity stuff first and send the refactored patch later.

--
Jonah H. Harris, Database Internals Architect
EnterpriseDB Corporation
732.331.1324


Re: InsertXLogFile in pg_resetxlog

From
Bruce Momjian
Date:
Jonah H. Harris wrote:
> On 5/6/06, Martijn van Oosterhout <kleptog@svana.org> wrote:
> > I've not seen any patch for this come past...
> 
> Yes, I got a little busy.  I ended up refactoring a good amount of the
> code because the entire thing is a little ugly.  I'll go ahead and
> just fix the Coverity stuff first and send the refactored patch later.

Jonah, it doesn't have to be 100% cleaned up, but if you can fix the
actual bugs, and clean up 50% of it, it is better than doing just the
bug fixes.

--  Bruce Momjian   http://candle.pha.pa.us EnterpriseDB    http://www.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +