Thread: 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@svana.org> http://svana.org/kleptog/ > From each according to his ability. To each according to his ability to litigate.
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
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
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
"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
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
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@svana.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@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
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. +