InsertXLogFile in pg_resetxlog

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(at)svana(dot)org> http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

Martijn van Oosterhout <kleptog(at)svana(dot)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

On 5/1/06, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

On 5/1/06, Jonah H. Harris <jonah(dot)harris(at)gmail(dot)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

"Jonah H. Harris" <jonah(dot)harris(at)gmail(dot)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

On 5/1/06, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

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

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(at)svana(dot)org> http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

On 5/6/06, Martijn van Oosterhout <kleptog(at)svana(dot)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

Jonah H. Harris wrote:
> On 5/6/06, Martijn van Oosterhout <kleptog(at)svana(dot)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. +